From: Walter Wu <walter-zh.wu@mediatek.com>
To: Thomas Gleixner <tglx@linutronix.de>
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@googlegroups.com>, <linux-mm@kvack.org>,
<linux-kernel@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>,
wsd_upstream <wsd_upstream@mediatek.com>,
<linux-mediatek@lists.infradead.org>
Subject: Re: [PATCH 1/5] timer: kasan: record and print timer stack
Date: Thu, 13 Aug 2020 20:48:57 +0800 [thread overview]
Message-ID: <1597322937.9999.42.camel@mtksdccf07> (raw)
In-Reply-To: <87d03ulqbp.fsf@nanos.tec.linutronix.de>
Hi Thomas,
Please ignore my previous mail. Thanks.
On Thu, 2020-08-13 at 13:48 +0200, Thomas Gleixner wrote:
> Walter,
>
> Walter Wu <walter-zh.wu@mediatek.com> writes:
> > This patch records the last two timer queueing stacks and prints
>
> "This patch" is useless information as we already know from the subject
> line that this is a patch.
>
> git grep 'This patch' Documentation/process/
>
Thanks for your information.
> > 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.
>
> we store nothing. Don't impersonate code please.
>
> Also please structure the changelog in a way that it's easy to
> understand what this is about instead of telling first what the patch
> does and then some half baken information why this is useful followed by
> more information about what it does.
>
> Something like this:
>
> For analysing use after free or double free of objects it is helpful
> to preserve usage history which potentially gives a hint about the
> affected code.
>
> For timers it has turned out to be useful to record the stack trace
> of the timer init call. <ADD technical explanation why this is useful>
>
> Record the most recent two timer init calls in KASAN which are printed
> on failure in the KASAN report.
>
> See, this gives a clear context, an explanation why it is useful and a
> high level description of what it does. The details are in the patch
> ifself and do not have to be epxlained in the changelog.
>
Thanks for your explanation, Our patch will use this as a template from
now on.
> For the technical explanation which you need to add, you really need to
> tell what's the advantage or additional coverage vs. existing debug
> facilities like debugobjects. Just claiming that it's useful does not
> make an argument.
>
We originally wanted him to have similar functions. Maybe he can't
completely replace, but KASAN can ave this ability.
> The UAF problem with timers is nasty because if you free an active timer
> then either the softirq which expires the timer will corrupt potentially
> reused memory or the reuse will corrupt the linked list which makes the
> softirq or some unrelated code which adds/removes a different timer
> explode in undebuggable ways. debugobject prevents that because it
> tracks per timer state and invokes the fixup function which keeps the
> system alive and also tells you exactly where the free of the active
> object happens which is the really interesting place to look at. The
> init function is pretty uninteresting in that case because you really
> want to know where the freeing of the active object happens.
>
> So if KASAN detects UAF in the timer softirq then the init trace is not
> giving any information especially not in cases where the timer is part
> of a common and frequently allocated/freed other data structure.
>
I don't have experience using this tool, but I will survey it.
> > 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) {}
>
> Duh, so you are adding per object type functions and storage? That's
> going to be a huge copy and pasta orgy as every object requires the same
> code and extra storage space.
>
> Why not just using kasan_record_aux_stack() for all of this?
>
> The 'call_rcu' 'timer' 'whatever next' printout is not really required
> because the stack trace already tells you the function which was
> invoked. If TOS is call_rcu() or do_timer_init() then it's entirely
> clear which object is affected. If the two aux records are not enough
> then making the array larger is not the end of the world.
>
My previous mail say that we will re-use kasan_record_aux_stack() and
only have aux_stack.
> > #endif /* CONFIG_KASAN_GENERIC */
> >
> > diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> > index a5221abb4594..ef2da9ddfac7 100644
> > --- a/kernel/time/timer.c
> > +++ b/kernel/time/timer.c
> > @@ -783,6 +783,8 @@ static void do_init_timer(struct timer_list *timer,
> > timer->function = func;
> > timer->flags = flags | raw_smp_processor_id();
> > lockdep_init_map(&timer->lockdep_map, name, key, 0);
> > +
> > + kasan_record_tmr_stack(timer);
> > }
>
> Are you sure this is correct for all timers?
>
> This is also called for timers which are temporarily allocated on stack
> and for timers which are statically allocated at compile time. How is
> that supposed to work?
>
If I understand correctly, KASAN report have this record only for slub
variable. So what you said shouldn't be a problem.
> These kind of things want to be explained upfront an not left to the
> reviewer as an exercise.
>
Sorry, My fault. Later we will be more cautious to send patch.
> Thanks,
>
> tglx
prev parent reply other threads:[~2020-08-13 12:49 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
2020-08-13 11:48 ` Thomas Gleixner
2020-08-13 12:25 ` Walter Wu
2020-08-13 12:48 ` Walter Wu [this message]
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=1597322937.9999.42.camel@mtksdccf07 \
--to=walter-zh.wu@mediatek.com \
--cc=akpm@linux-foundation.org \
--cc=aryabinin@virtuozzo.com \
--cc=dvyukov@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).