lkml.org 
[lkml]   [2020]   [Mar]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH -next] signal: annotate data races in sys_rt_sigaction
From
Date
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.

\
 
 \ /
  Last update: 2020-03-03 20:04    [W:0.148 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site