Skip to contentSkip to conversation

mm/vmalloc.c: Add an error message if two areas overlap

PATCH Superseded Uladzislau Rezki (Sony) 2 participants 7 messages Jul 10, 2020

Subsystems (2)
Memory management vmalloc

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Uladzislau Rezki (Sony)

author maintainer
#0 lore raw

PATCH 1/1 mm/vmalloc.c: Add an error message if two areas overlap

Before triggering a BUG() it would be useful to understand how two areas overlap between each other. Print information about start/end addresses of both VAs and their addresses.

For example if both are identical it could mean double free.

Signed-off-by Uladzislau Rezki (Sony) <[email protected]>
---
 mm/vmalloc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
mm/vmalloc.c +9 -1
index 5a2b55c8dd9a..1679b01febcd 100644
@@ -549,8 +549,13 @@ find_va_links(struct vmap_area *va,
 		else if (va->va_end > tmp_va->va_start &&
 				va->va_start >= tmp_va->va_end)
 			link = &(*link)->rb_right;
-		else
+		else {
+			pr_err("Overlaps: 0x%px(0x%lx-0x%lx), 0x%px(0x%lx-0x%lx)\n",
+				va, va->va_start, va->va_end, tmp_va,
+				tmp_va->va_start, tmp_va->va_end);
+
 			BUG();
+		}
 	} while (*link);
 
 	*parent = &tmp_va->rb_node;

@@ -1993,6 +1998,9 @@ static void vmap_init_free_space(void)
 			insert_vmap_area_augment(free, NULL,
 				&free_vmap_area_root,
 					&free_vmap_area_list);
+			insert_vmap_area_augment(free, NULL,
+				&free_vmap_area_root,
+					&free_vmap_area_list);
 		}
 	}
 }
Signature · 1 line
2.20.1
author maintainer
#1 lore raw
Quoted 32 lines

Before triggering a BUG() it would be useful to understand how two areas overlap between each other. Print information about start/end addresses of both VAs and their addresses.

For example if both are identical it could mean double free.

Signed-off-by Uladzislau Rezki (Sony) <[email protected]>
---
 mm/vmalloc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
mm/vmalloc.c +6 -1
index 5a2b55c8dd9a..1679b01febcd 100644
@@ -549,8 +549,13 @@ find_va_links(struct vmap_area *va,
 		else if (va->va_end > tmp_va->va_start &&
 				va->va_start >= tmp_va->va_end)
 			link = &(*link)->rb_right;
-		else
+		else {
+			pr_err("Overlaps: 0x%px(0x%lx-0x%lx), 0x%px(0x%lx-0x%lx)\n",
+				va, va->va_start, va->va_end, tmp_va,
+				tmp_va->va_start, tmp_va->va_end);
+
 			BUG();
+		}
 	} while (*link);
 
 	*parent = &tmp_va->rb_node;

mm/vmalloc.c static void vmap_init_free_space(void) · line 2006

Quoted 9 lines
mm/vmalloc.c +3 -0
@@ -1993,6 +1998,9 @@ static void vmap_init_free_space(void)
 			insert_vmap_area_augment(free, NULL,
 				&free_vmap_area_root,
 					&free_vmap_area_list);
+			insert_vmap_area_augment(free, NULL,
+				&free_vmap_area_root,
+					&free_vmap_area_list);
 		}
 	}
 }

Please ignore this patch, i will upload v2.

Sorry for inconvenience.

--
Vlad Rezki

#2 lore raw
In reply to Uladzislau Rezki (Sony)
Quoted 16 lines · Uladzislau Rezki (Sony)

Before triggering a BUG() it would be useful to understand how two areas overlap between each other. Print information about start/end addresses of both VAs and their addresses.

For example if both are identical it could mean double free.

Signed-off-by Uladzislau Rezki (Sony) <[email protected]>
---
 mm/vmalloc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
mm/vmalloc.c +0 -0
index 5a2b55c8dd9a..1679b01febcd 100644

mm/vmalloc.c find_va_links(struct vmap_area *va, · line 555

Quoted 8 lines
mm/vmalloc.c +4 -1
@@ -549,8 +549,13 @@ find_va_links(struct vmap_area *va,
 		else if (va->va_end > tmp_va->va_start &&
 				va->va_start >= tmp_va->va_end)
 			link = &(*link)->rb_right;
-		else
+		else {
+			pr_err("Overlaps: 0x%px(0x%lx-0x%lx), 0x%px(0x%lx-0x%lx)\n",
+				va, va->va_start, va->va_end, tmp_va,
+				tmp_va->va_start, tmp_va->va_end);

It might be helpful to have a "vmalloc:" prefix to that string to indicate where to start searching. And I don't think we're supposed to use %px without a really good justification these days.

Can we do without the BUG()? Maybe just break out and decline to insert the conflicting range?

mm/vmalloc.c static void vmap_init_free_space(void) · line 2003

Quoted 12 lines
mm/vmalloc.c +4 -0
 			BUG();
+		}
 	} while (*link);
 
 	*parent = &tmp_va->rb_node;

@@ -1993,6 +1998,9 @@ static void vmap_init_free_space(void)
 			insert_vmap_area_augment(free, NULL,
 				&free_vmap_area_root,
 					&free_vmap_area_list);
+			insert_vmap_area_augment(free, NULL,
+				&free_vmap_area_root,
+					&free_vmap_area_list);

This is surely testing code that you forgot to delete?

author maintainer
#3 lore raw
Quoted 37 lines · Matthew Wilcox
Quoted 16 lines · Uladzislau Rezki (Sony)

Before triggering a BUG() it would be useful to understand how two areas overlap between each other. Print information about start/end addresses of both VAs and their addresses.

For example if both are identical it could mean double free.

Signed-off-by Uladzislau Rezki (Sony) <[email protected]>
---
 mm/vmalloc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
mm/vmalloc.c +0 -0
index 5a2b55c8dd9a..1679b01febcd 100644

mm/vmalloc.c find_va_links(struct vmap_area *va, · line 555

Quoted 8 lines
mm/vmalloc.c +4 -1
@@ -549,8 +549,13 @@ find_va_links(struct vmap_area *va,
 		else if (va->va_end > tmp_va->va_start &&
 				va->va_start >= tmp_va->va_end)
 			link = &(*link)->rb_right;
-		else
+		else {
+			pr_err("Overlaps: 0x%px(0x%lx-0x%lx), 0x%px(0x%lx-0x%lx)\n",
+				va, va->va_start, va->va_end, tmp_va,
+				tmp_va->va_start, tmp_va->va_end);

It might be helpful to have a "vmalloc:" prefix to that string to indicate where to start searching. And I don't think we're supposed to use %px without a really good justification these days.

Can we do without the BUG()? Maybe just break out and decline to insert the conflicting range?

Quoted 4 lines
mm/vmalloc.c +1 -0
 			BUG();
+		}
 	} while (*link);
Quoted 10 lines

mm/vmalloc.c static void vmap_init_free_space(void) · line 2003

Quoted 8 lines
mm/vmalloc.c +3 -0
 	*parent = &tmp_va->rb_node;

@@ -1993,6 +1998,9 @@ static void vmap_init_free_space(void)
 			insert_vmap_area_augment(free, NULL,
 				&free_vmap_area_root,
 					&free_vmap_area_list);
+			insert_vmap_area_augment(free, NULL,
+				&free_vmap_area_root,
+					&free_vmap_area_list);

This is surely testing code that you forgot to delete?

Matthew, you are super fast!!! :)

--
Vlad Rezki

author maintainer
#4 lore raw
In reply to Matthew Wilcox
Quoted 21 lines · Matthew Wilcox
Quoted 20 lines · Uladzislau Rezki (Sony)

Before triggering a BUG() it would be useful to understand how two areas overlap between each other. Print information about start/end addresses of both VAs and their addresses.

For example if both are identical it could mean double free.

Signed-off-by Uladzislau Rezki (Sony) <[email protected]>
---
 mm/vmalloc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
mm/vmalloc.c +0 -1
index 5a2b55c8dd9a..1679b01febcd 100644
@@ -549,8 +549,13 @@ find_va_links(struct vmap_area *va,
 		else if (va->va_end > tmp_va->va_start &&
 				va->va_start >= tmp_va->va_end)
 			link = &(*link)->rb_right;
-		else
Quoted 9 lines

mm/vmalloc.c find_va_links(struct vmap_area *va, · line 555

Quoted 4 lines
mm/vmalloc.c +4 -0
+		else {
+			pr_err("Overlaps: 0x%px(0x%lx-0x%lx), 0x%px(0x%lx-0x%lx)\n",
+				va, va->va_start, va->va_end, tmp_va,
+				tmp_va->va_start, tmp_va->va_end);

It might be helpful to have a "vmalloc:" prefix to that string to indicate where to start searching. And I don't think we're supposed to use %px without a really good justification these days.

That makes sense, i will add such prefix for sure. As for %px i can just use a casting to (unsigned long) and 0x%lx prefix.

Quoted 4 lines

Can we do without the BUG()? Maybe just break out and decline to insert the conflicting range?

When i developed that logic, i was thinking about that but for some reason i went with BUG_ON(), do not remember why. But i agree we shold avoid of it. Let me think about it one more time.

Quoted 4 lines
Quoted 4 lines
mm/vmalloc.c +1 -0
 			BUG();
+		}
 	} while (*link);
Quoted 11 lines

mm/vmalloc.c static void vmap_init_free_space(void) · line 2003

Quoted 8 lines
mm/vmalloc.c +3 -0
 	*parent = &tmp_va->rb_node;

@@ -1993,6 +1998,9 @@ static void vmap_init_free_space(void)
 			insert_vmap_area_augment(free, NULL,
 				&free_vmap_area_root,
 					&free_vmap_area_list);
+			insert_vmap_area_augment(free, NULL,
+				&free_vmap_area_root,
+					&free_vmap_area_list);

This is surely testing code that you forgot to delete?

Yes, i forgot to remove it.

Thanks for your comments!

--
Vlad Rezki

#5 lore raw
Quoted 31 lines · Uladzislau Rezki
Quoted 21 lines · Matthew Wilcox
Quoted 20 lines · Uladzislau Rezki (Sony)

Before triggering a BUG() it would be useful to understand how two areas overlap between each other. Print information about start/end addresses of both VAs and their addresses.

For example if both are identical it could mean double free.

Signed-off-by Uladzislau Rezki (Sony) <[email protected]>
---
 mm/vmalloc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
mm/vmalloc.c +0 -1
index 5a2b55c8dd9a..1679b01febcd 100644
@@ -549,8 +549,13 @@ find_va_links(struct vmap_area *va,
 		else if (va->va_end > tmp_va->va_start &&
 				va->va_start >= tmp_va->va_end)
 			link = &(*link)->rb_right;
-		else
Quoted 9 lines

mm/vmalloc.c find_va_links(struct vmap_area *va, · line 555

Quoted 4 lines
mm/vmalloc.c +4 -0
+		else {
+			pr_err("Overlaps: 0x%px(0x%lx-0x%lx), 0x%px(0x%lx-0x%lx)\n",
+				va, va->va_start, va->va_end, tmp_va,
+				tmp_va->va_start, tmp_va->va_end);

It might be helpful to have a "vmalloc:" prefix to that string to indicate where to start searching. And I don't think we're supposed to use %px without a really good justification these days.

Quoted 2 lines

That makes sense, i will add such prefix for sure. As for %px i can just use a casting to (unsigned long) and 0x%lx prefix.

That's not the point. Linus wants us to not display actual pointers to the user.

author maintainer
#6 lore raw
Quoted 32 lines · Matthew Wilcox
Quoted 31 lines · Uladzislau Rezki
Quoted 30 lines · Matthew Wilcox
Quoted 16 lines · Uladzislau Rezki (Sony)

Before triggering a BUG() it would be useful to understand how two areas overlap between each other. Print information about start/end addresses of both VAs and their addresses.

For example if both are identical it could mean double free.

Signed-off-by Uladzislau Rezki (Sony) <[email protected]>
---
 mm/vmalloc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
mm/vmalloc.c +0 -0
index 5a2b55c8dd9a..1679b01febcd 100644

mm/vmalloc.c find_va_links(struct vmap_area *va, · line 555

Quoted 8 lines
mm/vmalloc.c +4 -1
@@ -549,8 +549,13 @@ find_va_links(struct vmap_area *va,
 		else if (va->va_end > tmp_va->va_start &&
 				va->va_start >= tmp_va->va_end)
 			link = &(*link)->rb_right;
-		else
+		else {
+			pr_err("Overlaps: 0x%px(0x%lx-0x%lx), 0x%px(0x%lx-0x%lx)\n",
+				va, va->va_start, va->va_end, tmp_va,
+				tmp_va->va_start, tmp_va->va_end);

It might be helpful to have a "vmalloc:" prefix to that string to indicate where to start searching. And I don't think we're supposed to use %px without a really good justification these days.

Quoted 6 lines
Quoted 2 lines

That makes sense, i will add such prefix for sure. As for %px i can just use a casting to (unsigned long) and 0x%lx prefix.

That's not the point. Linus wants us to not display actual pointers to the user.

Ahh, Got it!

Then i will just remove information about addresses of two VAs. Hope that will work.

--
Vlad Rezki