Messages in this thread | | | Subject | Re: [PATCH -next] signal: annotate data races in sys_rt_sigaction | From | Qian Cai <> | Date | Tue, 03 Mar 2020 14:01:03 -0500 |
| |
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.
| |