Message ID | 20200710194042.2510-1-urezki@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [1/1] mm/vmalloc.c: Add an error message if two areas overlap | expand |
> 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) <urezki@gmail.com> > --- > mm/vmalloc.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index 5a2b55c8dd9a..1679b01febcd 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -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); > } > } > } > Please ignore this patch, i will upload v2. Sorry for inconvenience. -- Vlad Rezki
On Fri, Jul 10, 2020 at 09:40:42PM +0200, Uladzislau Rezki (Sony) wrote: > 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) <urezki@gmail.com> > --- > mm/vmalloc.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index 5a2b55c8dd9a..1679b01febcd 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -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? > 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?
On Fri, Jul 10, 2020 at 08:44:06PM +0100, Matthew Wilcox wrote: > On Fri, Jul 10, 2020 at 09:40:42PM +0200, Uladzislau Rezki (Sony) wrote: > > 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) <urezki@gmail.com> > > --- > > mm/vmalloc.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > > index 5a2b55c8dd9a..1679b01febcd 100644 > > --- a/mm/vmalloc.c > > +++ b/mm/vmalloc.c > > @@ -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? > > > 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? Matthew, you are super fast!!! :) -- Vlad Rezki
On Fri, Jul 10, 2020 at 08:44:06PM +0100, Matthew Wilcox wrote: > On Fri, Jul 10, 2020 at 09:40:42PM +0200, Uladzislau Rezki (Sony) wrote: > > 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) <urezki@gmail.com> > > --- > > mm/vmalloc.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > > index 5a2b55c8dd9a..1679b01febcd 100644 > > --- a/mm/vmalloc.c > > +++ b/mm/vmalloc.c > > @@ -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. > 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. > > 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. > > 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? > Yes, i forgot to remove it. Thanks for your comments! -- Vlad Rezki
On Fri, Jul 10, 2020 at 10:00:30PM +0200, Uladzislau Rezki wrote: > On Fri, Jul 10, 2020 at 08:44:06PM +0100, Matthew Wilcox wrote: > > On Fri, Jul 10, 2020 at 09:40:42PM +0200, Uladzislau Rezki (Sony) wrote: > > > 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) <urezki@gmail.com> > > > --- > > > mm/vmalloc.c | 10 +++++++++- > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > > > index 5a2b55c8dd9a..1679b01febcd 100644 > > > --- a/mm/vmalloc.c > > > +++ b/mm/vmalloc.c > > > @@ -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. > > > 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.
On Fri, Jul 10, 2020 at 09:01:38PM +0100, Matthew Wilcox wrote: > On Fri, Jul 10, 2020 at 10:00:30PM +0200, Uladzislau Rezki wrote: > > On Fri, Jul 10, 2020 at 08:44:06PM +0100, Matthew Wilcox wrote: > > > On Fri, Jul 10, 2020 at 09:40:42PM +0200, Uladzislau Rezki (Sony) wrote: > > > > 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) <urezki@gmail.com> > > > > --- > > > > mm/vmalloc.c | 10 +++++++++- > > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > > > > index 5a2b55c8dd9a..1679b01febcd 100644 > > > > --- a/mm/vmalloc.c > > > > +++ b/mm/vmalloc.c > > > > @@ -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. > > > > > 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
diff --git a/mm/vmalloc.c b/mm/vmalloc.c index 5a2b55c8dd9a..1679b01febcd 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -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); } } }
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) <urezki@gmail.com> --- mm/vmalloc.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)