From: Walter Wu <walter-zh.wu@mediatek.com>
To: Marco Elver <elver@google.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>,
Alexander Potapenko <glider@google.com>,
Dmitry Vyukov <dvyukov@google.com>,
Matthias Brugger <matthias.bgg@gmail.com>,
John Stultz <john.stultz@linaro.org>,
"Stephen Boyd" <sboyd@kernel.org>,
Andrew Morton <akpm@linux-foundation.org>,
kasan-dev <kasan-dev@googlegroups.com>,
Linux Memory Management List <linux-mm@kvack.org>,
LKML <linux-kernel@vger.kernel.org>,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
wsd_upstream <wsd_upstream@mediatek.com>,
<linux-mediatek@lists.infradead.org>,
"Thomas Gleixner" <tglx@linutronix.de>
Subject: Re: [PATCH 1/5] timer: kasan: record and print timer stack
Date: Thu, 13 Aug 2020 09:46:18 +0800 [thread overview]
Message-ID: <1597283178.9999.19.camel@mtksdccf07> (raw)
In-Reply-To: <CANpmjNO9=JBcSV-nif9a=4Zt7gTCp6e5c2jVXMCSFgP3v2P9-w@mail.gmail.com>
On Wed, 2020-08-12 at 16:13 +0200, Marco Elver wrote:
> On Mon, 10 Aug 2020 at 09:23, Walter Wu <walter-zh.wu@mediatek.com> wrote:
> > This patch records the last two timer queueing stacks and prints
> > up to 2 timer stacks in KASAN report. It is useful for programmers
> > to solve use-after-free or double-free memory timer issues.
> >
> > When timer_setup() or timer_setup_on_stack() is called, then it
> > prepares to use this timer and sets timer callback, we store
> > this call stack in order to print it in KASAN report.
> >
> > Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
> > Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> > Cc: Dmitry Vyukov <dvyukov@google.com>
> > Cc: Alexander Potapenko <glider@google.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: John Stultz <john.stultz@linaro.org>
> > Cc: Stephen Boyd <sboyd@kernel.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > ---
> > include/linux/kasan.h | 2 ++
> > kernel/time/timer.c | 2 ++
> > mm/kasan/generic.c | 21 +++++++++++++++++++++
> > mm/kasan/kasan.h | 4 +++-
> > mm/kasan/report.c | 11 +++++++++++
> > 5 files changed, 39 insertions(+), 1 deletion(-)
>
> I'm commenting on the code here, but it also applies to patch 2/5, as
> it's almost a copy-paste.
>
> In general, I'd say the solution to get this feature is poorly
> designed, resulting in excessive LOC added. The logic added already
> exists for the aux stacks.
>
That's true, we will have refactoring for it.
> > diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> > index 23b7ee00572d..763664b36dc6 100644
> > --- a/include/linux/kasan.h
> > +++ b/include/linux/kasan.h
> > @@ -175,12 +175,14 @@ static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; }
> > void kasan_cache_shrink(struct kmem_cache *cache);
> > void kasan_cache_shutdown(struct kmem_cache *cache);
> > void kasan_record_aux_stack(void *ptr);
> > +void kasan_record_tmr_stack(void *ptr);
> >
> > #else /* CONFIG_KASAN_GENERIC */
> >
> > static inline void kasan_cache_shrink(struct kmem_cache *cache) {}
> > static inline void kasan_cache_shutdown(struct kmem_cache *cache) {}
> > static inline void kasan_record_aux_stack(void *ptr) {}
> > +static inline void kasan_record_tmr_stack(void *ptr) {}
>
> It appears that the 'aux' stack is currently only used for call_rcu
> stacks, but this interface does not inherently tie it to call_rcu. The
> only thing tying it to call_rcu is the fact that the report calls out
> call_rcu.
>
> > /**
> > diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> > index 4b3cbad7431b..f35dcec990ab 100644
> > --- a/mm/kasan/generic.c
> > +++ b/mm/kasan/generic.c
> > @@ -347,6 +347,27 @@ void kasan_record_aux_stack(void *addr)
> > alloc_info->aux_stack[0] = kasan_save_stack(GFP_NOWAIT);
> > }
> >
> > +void kasan_record_tmr_stack(void *addr)
> > +{
> > + struct page *page = kasan_addr_to_page(addr);
> > + struct kmem_cache *cache;
> > + struct kasan_alloc_meta *alloc_info;
> > + void *object;
> > +
> > + if (!(page && PageSlab(page)))
> > + return;
> > +
> > + cache = page->slab_cache;
> > + object = nearest_obj(cache, page, addr);
> > + alloc_info = get_alloc_info(cache, object);
> > +
> > + /*
> > + * record the last two timer stacks.
> > + */
> > + alloc_info->tmr_stack[1] = alloc_info->tmr_stack[0];
> > + alloc_info->tmr_stack[0] = kasan_save_stack(GFP_NOWAIT);
> > +}
>
> The solution here is, unfortunately, poorly designed. This is a
> copy-paste of the kasan_record_aux_stack() function.
>
kasan_record_aux_stack() will be re-used for call_rcu, timer, and
workqueue.
> > void kasan_set_free_info(struct kmem_cache *cache,
> > void *object, u8 tag)
> > {
> > diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> > index ef655a1c6e15..c50827f388a3 100644
> > --- a/mm/kasan/kasan.h
> > +++ b/mm/kasan/kasan.h
> > @@ -108,10 +108,12 @@ struct kasan_alloc_meta {
> > struct kasan_track alloc_track;
> > #ifdef CONFIG_KASAN_GENERIC
> > /*
> > - * call_rcu() call stack is stored into struct kasan_alloc_meta.
> > + * call_rcu() call stack and timer queueing stack are stored
> > + * into struct kasan_alloc_meta.
> > * The free stack is stored into struct kasan_free_meta.
> > */
> > depot_stack_handle_t aux_stack[2];
> > + depot_stack_handle_t tmr_stack[2];
> > #else
> > struct kasan_track free_track[KASAN_NR_FREE_STACKS];
> > #endif
> > diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> > index fed3c8fdfd25..6fa3bfee381f 100644
> > --- a/mm/kasan/report.c
> > +++ b/mm/kasan/report.c
> > @@ -191,6 +191,17 @@ static void describe_object(struct kmem_cache *cache, void *object,
> > print_stack(alloc_info->aux_stack[1]);
> > pr_err("\n");
> > }
> > +
> > + if (alloc_info->tmr_stack[0]) {
> > + pr_err("Last timer stack:\n");
> > + print_stack(alloc_info->tmr_stack[0]);
> > + pr_err("\n");
> > + }
> > + if (alloc_info->tmr_stack[1]) {
> > + pr_err("Second to last timer stack:\n");
> > + print_stack(alloc_info->tmr_stack[1]);
> > + pr_err("\n");
> > + }
>
> Why can't we just use the aux stack for everything, and simply change
> the message printed in the report. After all, the stack trace will
> include all the information to tell if it's call_rcu, timer, or
> workqueue.
>
This is a good suggestion, next patch will do it.
> The reporting code would simply have to be changed to say something
> like "Last potentially related work creation:" -- because what the
> "aux" thing is trying to abstract are stack traces to work creations
> that took an address that is closely related. Whether or not you want
> to call it "work" is up to you, but that's the most generic term I
> could think of right now (any better terms?).
>
Work is good.
> Another argument for this consolidation is that it's highly unlikely
> that aux_stack[a] && tmr_stack[b] && wq_stack[c], and you need to
> print all the stacks. If you are worried we need more aux stacks, just
> make the array size 3+ (but I think it's not necessary).
>
We hope that next patch keep it as it is aux_stack[2], it may record
call_rcu, timer, or workqueue. Because struct kasan_alloc_meta is 16
bytes, it will have the minimal redzone size and good alignment, lets
get better memory consumption.
Thanks for your good suggestion.
Walter
> Thanks,
> -- Marco
next prev parent reply other threads:[~2020-08-13 1:46 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-10 7:23 [PATCH 1/5] timer: kasan: record and print timer stack Walter Wu
2020-08-12 14:13 ` Marco Elver
2020-08-13 1:46 ` Walter Wu [this message]
2020-08-13 11:48 ` Thomas Gleixner
2020-08-13 12:25 ` Walter Wu
2020-08-13 12:48 ` Walter Wu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1597283178.9999.19.camel@mtksdccf07 \
--to=walter-zh.wu@mediatek.com \
--cc=akpm@linux-foundation.org \
--cc=aryabinin@virtuozzo.com \
--cc=dvyukov@google.com \
--cc=elver@google.com \
--cc=glider@google.com \
--cc=john.stultz@linaro.org \
--cc=kasan-dev@googlegroups.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-mm@kvack.org \
--cc=matthias.bgg@gmail.com \
--cc=sboyd@kernel.org \
--cc=tglx@linutronix.de \
--cc=wsd_upstream@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).