Re: [PATCH -next] signal: annotate data races in sys_rt_sigaction

[Qian Cai <cai@lca.pw>]

On Tue, 2020-03-03 at 19:26 +0100, Marco Elver wrote:
> On Tue, 3 Mar 2020 at 18:53, Marco Elver <elver@google.com> wrote:
> > 
> > On Tue, 3 Mar 2020 at 18:21, Qian Cai <cai@lca.pw> wrote:
> > > 
> > > Kmemleak could scan task stacks while plain writes happens to those
> > > stack variables which could results in data races. For example, in
> > > sys_rt_sigaction and do_sigaction(), it could have plain writes in
> > > a 32-byte size. Since the kmemleak does not care about the actual values
> > > of a non-pointer and all do_sigaction() call sites only copy to stack
> > > variables, annotate them as intentional data races using the
> > > data_race() macro. The data races were reported by KCSAN,
> > > 
> > >  BUG: KCSAN: data-race in _copy_from_user / scan_block
> > > 
> > >  read to 0xffffb3074e61fe58 of 8 bytes by task 356 on cpu 19:
> > >   scan_block+0x6e/0x1a0
> > >   scan_block at mm/kmemleak.c:1251
> > >   kmemleak_scan+0xbea/0xd20
> > >   kmemleak_scan at mm/kmemleak.c:1482
> > >   kmemleak_scan_thread+0xcc/0xfa
> > >   kthread+0x1cd/0x1f0
> > >   ret_from_fork+0x3a/0x50
> > 
> > I think we should move the annotations to kmemleak instead of signal.c.
> > 
> > Because putting a "data_race()" on the accesses in signal.c just
> > because of Kmemleak feels wrong because then we might miss other more
> > serious issues. Kmemleak isn't normally enabled in a non-debug kernel.
> > 
> > I wonder if it'd be a better idea to just disable KCSAN on scan_block
> > with __no_kcsan? If Kmemleak only does reads, then __no_kcsan will do
> > the right thing here, because the reads are hidden completely from
> > KCSAN. With "data_race()" you would still have to mark both accesses
> > in signal.c and kmemleak (this is by design, so that we document all
> > intentionally data-racy accesses).
> > 
> > An alternative would be to just exempt kmemleak from KCSAN with
> > "KCSAN_SANITIZE_kmemleak.o := n". Given Kmemleak is a debugging tool
> > and it's expected to race with all kinds of accesses, maybe that's the
> > best option.
> 
> I saw there are already some data_race() annotations in Kmemleak.
> Given there are probably more things waiting to be found in Kmemleak,
> KCSAN_SANITIZE_kmemleak.o := n might just be the best option. I think
> this is fair, because we really do not want to annotate anything
> outside Kmemleak just because Kmemleak scans everything. The existing
> annotations should probably be reverted in that case.

Good idea. I'll post a new patch for that.