diff mbox series

[v4,3/6] kasan: print timer and workqueue stack

Message ID 20200924040513.31051-1-walter-zh.wu@mediatek.com
State New
Headers show
Series kasan: add workqueue and timer stack for generic KASAN | expand

Commit Message

Walter Wu Sept. 24, 2020, 4:05 a.m. UTC
The aux_stack[2] is reused to record the call_rcu() call stack,
timer init call stack, and enqueuing work call stacks. So that
we need to change the auxiliary stack title for common title,
print them in KASAN report.

Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
Suggested-by: Marco Elver <elver@google.com>
Acked-by: Marco Elver <elver@google.com>
Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
Reviewed-by: Andrey Konovalov <andreyknvl@google.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Alexander Potapenko <glider@google.com>
---

v2:
- Thanks for Marco suggestion.
- We modify aux stack title name in KASAN report
  in order to print call_rcu()/timer/workqueue stack.

---
 mm/kasan/report.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Alexander Potapenko Sept. 24, 2020, 11:47 a.m. UTC | #1
On Thu, Sep 24, 2020 at 6:05 AM Walter Wu <walter-zh.wu@mediatek.com> wrote:
>
> The aux_stack[2] is reused to record the call_rcu() call stack,
> timer init call stack, and enqueuing work call stacks. So that
> we need to change the auxiliary stack title for common title,
> print them in KASAN report.
>
> Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
> Suggested-by: Marco Elver <elver@google.com>
> Acked-by: Marco Elver <elver@google.com>
> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
> Reviewed-by: Andrey Konovalov <andreyknvl@google.com>
> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: Alexander Potapenko <glider@google.com>
> ---
>
> v2:
> - Thanks for Marco suggestion.
> - We modify aux stack title name in KASAN report
>   in order to print call_rcu()/timer/workqueue stack.
>
> ---
>  mm/kasan/report.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index 4f49fa6cd1aa..886809d0a8dd 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -183,12 +183,12 @@ static void describe_object(struct kmem_cache *cache, void *object,
>
>  #ifdef CONFIG_KASAN_GENERIC
>                 if (alloc_info->aux_stack[0]) {
> -                       pr_err("Last call_rcu():\n");
> +                       pr_err("Last potentially related work creation:\n");

This doesn't have to be a work creation (expect more callers of
kasan_record_aux_stack() in the future), so maybe change the wording
here to "Last potentially related auxiliary stack"?
Marco Elver Sept. 24, 2020, 11:54 a.m. UTC | #2
On Thu, 24 Sep 2020 at 13:47, Alexander Potapenko <glider@google.com> wrote:
>
> On Thu, Sep 24, 2020 at 6:05 AM Walter Wu <walter-zh.wu@mediatek.com> wrote:
> >
> > The aux_stack[2] is reused to record the call_rcu() call stack,
> > timer init call stack, and enqueuing work call stacks. So that
> > we need to change the auxiliary stack title for common title,
> > print them in KASAN report.
> >
> > Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
> > Suggested-by: Marco Elver <elver@google.com>
> > Acked-by: Marco Elver <elver@google.com>
> > Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
> > Reviewed-by: Andrey Konovalov <andreyknvl@google.com>
> > Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> > Cc: Alexander Potapenko <glider@google.com>
> > ---
> >
> > v2:
> > - Thanks for Marco suggestion.
> > - We modify aux stack title name in KASAN report
> >   in order to print call_rcu()/timer/workqueue stack.
> >
> > ---
> >  mm/kasan/report.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> > index 4f49fa6cd1aa..886809d0a8dd 100644
> > --- a/mm/kasan/report.c
> > +++ b/mm/kasan/report.c
> > @@ -183,12 +183,12 @@ static void describe_object(struct kmem_cache *cache, void *object,
> >
> >  #ifdef CONFIG_KASAN_GENERIC
> >                 if (alloc_info->aux_stack[0]) {
> > -                       pr_err("Last call_rcu():\n");
> > +                       pr_err("Last potentially related work creation:\n");
>
> This doesn't have to be a work creation (expect more callers of
> kasan_record_aux_stack() in the future), so maybe change the wording
> here to "Last potentially related auxiliary stack"?

I suggested "work creation" as it's the most precise for what it is
used for now.

What other users do you have in mind in future that are not work creation?
Alexander Potapenko Sept. 24, 2020, 12:11 p.m. UTC | #3
On Thu, Sep 24, 2020 at 1:55 PM Marco Elver <elver@google.com> wrote:
>
> On Thu, 24 Sep 2020 at 13:47, Alexander Potapenko <glider@google.com> wrote:
> >
> > On Thu, Sep 24, 2020 at 6:05 AM Walter Wu <walter-zh.wu@mediatek.com> wrote:
> > >
> > > The aux_stack[2] is reused to record the call_rcu() call stack,
> > > timer init call stack, and enqueuing work call stacks. So that
> > > we need to change the auxiliary stack title for common title,
> > > print them in KASAN report.
> > >
> > > Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
> > > Suggested-by: Marco Elver <elver@google.com>
> > > Acked-by: Marco Elver <elver@google.com>
> > > Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
> > > Reviewed-by: Andrey Konovalov <andreyknvl@google.com>
> > > Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> > > Cc: Alexander Potapenko <glider@google.com>
> > > ---
> > >
> > > v2:
> > > - Thanks for Marco suggestion.
> > > - We modify aux stack title name in KASAN report
> > >   in order to print call_rcu()/timer/workqueue stack.
> > >
> > > ---
> > >  mm/kasan/report.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> > > index 4f49fa6cd1aa..886809d0a8dd 100644
> > > --- a/mm/kasan/report.c
> > > +++ b/mm/kasan/report.c
> > > @@ -183,12 +183,12 @@ static void describe_object(struct kmem_cache *cache, void *object,
> > >
> > >  #ifdef CONFIG_KASAN_GENERIC
> > >                 if (alloc_info->aux_stack[0]) {
> > > -                       pr_err("Last call_rcu():\n");
> > > +                       pr_err("Last potentially related work creation:\n");
> >
> > This doesn't have to be a work creation (expect more callers of
> > kasan_record_aux_stack() in the future), so maybe change the wording
> > here to "Last potentially related auxiliary stack"?
>
> I suggested "work creation" as it's the most precise for what it is
> used for now.

I see, then maybe my suggestion is premature.

> What other users do you have in mind in future that are not work creation?

I think saving stacks may help in any case where an object is reused
for a different purpose without reallocation.
SKBs, maybe?


--
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Marco Elver Sept. 24, 2020, 12:21 p.m. UTC | #4
On Thu, 24 Sep 2020 at 14:11, Alexander Potapenko <glider@google.com> wrote:
>
> On Thu, Sep 24, 2020 at 1:55 PM Marco Elver <elver@google.com> wrote:
> >
> > On Thu, 24 Sep 2020 at 13:47, Alexander Potapenko <glider@google.com> wrote:
> > >
> > > On Thu, Sep 24, 2020 at 6:05 AM Walter Wu <walter-zh.wu@mediatek.com> wrote:
> > > >
> > > > The aux_stack[2] is reused to record the call_rcu() call stack,
> > > > timer init call stack, and enqueuing work call stacks. So that
> > > > we need to change the auxiliary stack title for common title,
> > > > print them in KASAN report.
> > > >
> > > > Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
> > > > Suggested-by: Marco Elver <elver@google.com>
> > > > Acked-by: Marco Elver <elver@google.com>
> > > > Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
> > > > Reviewed-by: Andrey Konovalov <andreyknvl@google.com>
> > > > Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> > > > Cc: Alexander Potapenko <glider@google.com>
> > > > ---
> > > >
> > > > v2:
> > > > - Thanks for Marco suggestion.
> > > > - We modify aux stack title name in KASAN report
> > > >   in order to print call_rcu()/timer/workqueue stack.
> > > >
> > > > ---
> > > >  mm/kasan/report.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> > > > index 4f49fa6cd1aa..886809d0a8dd 100644
> > > > --- a/mm/kasan/report.c
> > > > +++ b/mm/kasan/report.c
> > > > @@ -183,12 +183,12 @@ static void describe_object(struct kmem_cache *cache, void *object,
> > > >
> > > >  #ifdef CONFIG_KASAN_GENERIC
> > > >                 if (alloc_info->aux_stack[0]) {
> > > > -                       pr_err("Last call_rcu():\n");
> > > > +                       pr_err("Last potentially related work creation:\n");
> > >
> > > This doesn't have to be a work creation (expect more callers of
> > > kasan_record_aux_stack() in the future), so maybe change the wording
> > > here to "Last potentially related auxiliary stack"?
> >
> > I suggested "work creation" as it's the most precise for what it is
> > used for now.
>
> I see, then maybe my suggestion is premature.
>
> > What other users do you have in mind in future that are not work creation?
>
> I think saving stacks may help in any case where an object is reused
> for a different purpose without reallocation.
> SKBs, maybe?

I currently don't know, it's hard to say without having a report that
we can't debug without it.

The litmus test for if it's useful would probably be "do we need this
stacktrace to debug a use-after-free/double-free?". If the answer is
maybe (and not yes!), I'd err on the side of not going overboard with
these, because we only have limited storage anyway. "Work creation" is
a clear case of "we loose information to the original caller" and need
it to debug. But of course, if there are similar issues elsewhere, we
need to identify them and then decide if we need it.
diff mbox series

Patch

diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 4f49fa6cd1aa..886809d0a8dd 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -183,12 +183,12 @@  static void describe_object(struct kmem_cache *cache, void *object,
 
 #ifdef CONFIG_KASAN_GENERIC
 		if (alloc_info->aux_stack[0]) {
-			pr_err("Last call_rcu():\n");
+			pr_err("Last potentially related work creation:\n");
 			print_stack(alloc_info->aux_stack[0]);
 			pr_err("\n");
 		}
 		if (alloc_info->aux_stack[1]) {
-			pr_err("Second to last call_rcu():\n");
+			pr_err("Second to last potentially related work creation:\n");
 			print_stack(alloc_info->aux_stack[1]);
 			pr_err("\n");
 		}