| Message ID | 5b120f7cadcc0e0d8d5f41fd0cff35981b3f7f3a.1645038022.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [mm] kasan: print virtual mapping info in reports | expand |
On Wed, 16 Feb 2022 at 20:01, <andrey.konovalov@linux.dev> wrote: > > From: Andrey Konovalov <andreyknvl@google.com> > > Print virtual mapping range and its creator in reports affecting virtual > mappings. > > Also get physical page pointer for such mappings, so page information > gets printed as well. > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > --- > > Note: no need to merge this patch into any of the KASAN vmalloc patches > that are already in mm, better to keep it separate. > --- > mm/kasan/report.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/mm/kasan/report.c b/mm/kasan/report.c > index 137c2c0b09db..8002fb3c417d 100644 > --- a/mm/kasan/report.c > +++ b/mm/kasan/report.c > @@ -260,8 +260,18 @@ static void print_address_description(void *addr, u8 tag) > pr_err(" %pS\n", addr); > } > > + if (is_vmalloc_addr(addr)) { > + struct vm_struct *va = find_vm_area(addr); > + > + pr_err("The buggy address belongs to the virtual mapping at\n" > + " [%px, %px) created by:\n" > + " %pS\n", va->addr, va->addr + va->size, va->caller); Can you show an example of what this looks like? It's not showing a stack trace, so why not continue the line and just say "... created by: %pS\n" > + page = vmalloc_to_page(page); > + } > + > if (page) { > - pr_err("The buggy address belongs to the page:\n"); > + pr_err("The buggy address belongs to the physical page:\n"); > dump_page(page, "kasan: bad access detected"); > } > > -- > 2.25.1 >
On Wed, Feb 16, 2022 at 8:31 PM Marco Elver <elver@google.com> wrote: > > On Wed, 16 Feb 2022 at 20:01, <andrey.konovalov@linux.dev> wrote: > > > > From: Andrey Konovalov <andreyknvl@google.com> > > > > Print virtual mapping range and its creator in reports affecting virtual > > mappings. > > > > Also get physical page pointer for such mappings, so page information > > gets printed as well. > > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > > > --- > > > > Note: no need to merge this patch into any of the KASAN vmalloc patches > > that are already in mm, better to keep it separate. > > --- > > mm/kasan/report.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/mm/kasan/report.c b/mm/kasan/report.c > > index 137c2c0b09db..8002fb3c417d 100644 > > --- a/mm/kasan/report.c > > +++ b/mm/kasan/report.c > > @@ -260,8 +260,18 @@ static void print_address_description(void *addr, u8 tag) > > pr_err(" %pS\n", addr); > > } > > > > + if (is_vmalloc_addr(addr)) { > > + struct vm_struct *va = find_vm_area(addr); > > + > > + pr_err("The buggy address belongs to the virtual mapping at\n" > > + " [%px, %px) created by:\n" > > + " %pS\n", va->addr, va->addr + va->size, va->caller); > > Can you show an example of what this looks like? [ 20.883723] The buggy address belongs to the virtual mapping at [ 20.883723] [ffff8000081c9000, ffff8000081cb000) created by: [ 20.883723] vmalloc_oob+0xd8/0x4dc > It's not showing a stack trace, No, only a single frame. > so why not continue the line and just say "... created by: %pS\n" Putting it on a separate line makes the line lengths looks more balanced. Also, printing a frame on a separate line is consistent with the rest of KASAN reporting code.
On Wed, 16 Feb 2022 at 21:42, Andrey Konovalov <andreyknvl@gmail.com> wrote: > > On Wed, Feb 16, 2022 at 8:31 PM Marco Elver <elver@google.com> wrote: > > > > On Wed, 16 Feb 2022 at 20:01, <andrey.konovalov@linux.dev> wrote: > > > > > > From: Andrey Konovalov <andreyknvl@google.com> > > > > > > Print virtual mapping range and its creator in reports affecting virtual > > > mappings. > > > > > > Also get physical page pointer for such mappings, so page information > > > gets printed as well. > > > > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > > > > > --- > > > > > > Note: no need to merge this patch into any of the KASAN vmalloc patches > > > that are already in mm, better to keep it separate. > > > --- > > > mm/kasan/report.c | 12 +++++++++++- > > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > > > diff --git a/mm/kasan/report.c b/mm/kasan/report.c > > > index 137c2c0b09db..8002fb3c417d 100644 > > > --- a/mm/kasan/report.c > > > +++ b/mm/kasan/report.c > > > @@ -260,8 +260,18 @@ static void print_address_description(void *addr, u8 tag) > > > pr_err(" %pS\n", addr); > > > } > > > > > > + if (is_vmalloc_addr(addr)) { > > > + struct vm_struct *va = find_vm_area(addr); > > > + > > > + pr_err("The buggy address belongs to the virtual mapping at\n" > > > + " [%px, %px) created by:\n" > > > + " %pS\n", va->addr, va->addr + va->size, va->caller); > > > > Can you show an example of what this looks like? > > [ 20.883723] The buggy address belongs to the virtual mapping at > [ 20.883723] [ffff8000081c9000, ffff8000081cb000) created by: > [ 20.883723] vmalloc_oob+0xd8/0x4dc > > > It's not showing a stack trace, > > No, only a single frame. > > > so why not continue the line and just say "... created by: %pS\n" > > Putting it on a separate line makes the line lengths looks more balanced. > > Also, printing a frame on a separate line is consistent with the rest > of KASAN reporting code. That's reasonable, thanks. Reviewed-by: Marco Elver <elver@google.com>
On Wed, Feb 16, 2022 at 08:01:37PM +0100, andrey.konovalov@linux.dev wrote: > From: Andrey Konovalov <andreyknvl@google.com> > > Print virtual mapping range and its creator in reports affecting virtual > mappings. > > Also get physical page pointer for such mappings, so page information > gets printed as well. > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > --- > > Note: no need to merge this patch into any of the KASAN vmalloc patches > that are already in mm, better to keep it separate. > --- > mm/kasan/report.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/mm/kasan/report.c b/mm/kasan/report.c > index 137c2c0b09db..8002fb3c417d 100644 > --- a/mm/kasan/report.c > +++ b/mm/kasan/report.c > @@ -260,8 +260,18 @@ static void print_address_description(void *addr, u8 tag) > pr_err(" %pS\n", addr); > } > > + if (is_vmalloc_addr(addr)) { > + struct vm_struct *va = find_vm_area(addr); > + > + pr_err("The buggy address belongs to the virtual mapping at\n" > + " [%px, %px) created by:\n" > + " %pS\n", va->addr, va->addr + va->size, va->caller); The return value of find_vm_area() needs a NULL check here; is_vmalloc_addr(addr) just checks that `addr` is within the vmalloc VA range, and doesn't guarantee that there is a vmap_area associated with that `addr`. Without the NULL-check, we'll blow up on the `va->addr` dereference and will fail to make the report, which would be unfortunate. Thanks, Mark. > + > + page = vmalloc_to_page(page); > + } > + > if (page) { > - pr_err("The buggy address belongs to the page:\n"); > + pr_err("The buggy address belongs to the physical page:\n"); > dump_page(page, "kasan: bad access detected"); > } > > -- > 2.25.1 >
On Thu, Feb 17, 2022 at 1:00 PM Mark Rutland <mark.rutland@arm.com> wrote: > > On Wed, Feb 16, 2022 at 08:01:37PM +0100, andrey.konovalov@linux.dev wrote: > > From: Andrey Konovalov <andreyknvl@google.com> > > > > Print virtual mapping range and its creator in reports affecting virtual > > mappings. > > > > Also get physical page pointer for such mappings, so page information > > gets printed as well. > > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > > > --- > > > > Note: no need to merge this patch into any of the KASAN vmalloc patches > > that are already in mm, better to keep it separate. > > --- > > mm/kasan/report.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/mm/kasan/report.c b/mm/kasan/report.c > > index 137c2c0b09db..8002fb3c417d 100644 > > --- a/mm/kasan/report.c > > +++ b/mm/kasan/report.c > > @@ -260,8 +260,18 @@ static void print_address_description(void *addr, u8 tag) > > pr_err(" %pS\n", addr); > > } > > > > + if (is_vmalloc_addr(addr)) { > > + struct vm_struct *va = find_vm_area(addr); > > + > > + pr_err("The buggy address belongs to the virtual mapping at\n" > > + " [%px, %px) created by:\n" > > + " %pS\n", va->addr, va->addr + va->size, va->caller); > > The return value of find_vm_area() needs a NULL check here; > is_vmalloc_addr(addr) just checks that `addr` is within the vmalloc VA > range, and doesn't guarantee that there is a vmap_area associated with > that `addr`. > > Without the NULL-check, we'll blow up on the `va->addr` dereference and > will fail to make the report, which would be unfortunate. Indeed. Will fix in v2. Thanks, Mark!
diff --git a/mm/kasan/report.c b/mm/kasan/report.c index 137c2c0b09db..8002fb3c417d 100644 --- a/mm/kasan/report.c +++ b/mm/kasan/report.c @@ -260,8 +260,18 @@ static void print_address_description(void *addr, u8 tag) pr_err(" %pS\n", addr); } + if (is_vmalloc_addr(addr)) { + struct vm_struct *va = find_vm_area(addr); + + pr_err("The buggy address belongs to the virtual mapping at\n" + " [%px, %px) created by:\n" + " %pS\n", va->addr, va->addr + va->size, va->caller); + + page = vmalloc_to_page(page); + } + if (page) { - pr_err("The buggy address belongs to the page:\n"); + pr_err("The buggy address belongs to the physical page:\n"); dump_page(page, "kasan: bad access detected"); }