On Tue 17-06-25 14:04:37, Dmitry Antipov wrote: > Running both syzbot reproducers and even during regular system boots, > KCSAN is likely to report the data race around using 'triggered' flag > of 'struct poll_wqueues'. Suspected race may be either read vs. write > (observed locally during system boot): > > BUG: KCSAN: data-race in poll_schedule_timeout / pollwake > > write to 0xffffc90004397b90 of 4 bytes by task 5619 on cpu 4: > pollwake+0xd1/0x130 > __wake_up_common_lock+0x7f/0xd0 > sock_def_readable+0x20e/0x590 > unix_dgram_sendmsg+0xa3a/0x1050 > unix_seqpacket_sendmsg+0xdb/0x140 > __sock_sendmsg+0x151/0x190 > sock_write_iter+0x172/0x1c0 > vfs_write+0x66d/0x6f0 > ksys_write+0xe7/0x1b0 > __x64_sys_write+0x4a/0x60 > x64_sys_call+0x2f35/0x32b0 > do_syscall_64+0xfa/0x3b0 > entry_SYSCALL_64_after_hwframe+0x77/0x7f > > read to 0xffffc90004397b90 of 4 bytes by task 5620 on cpu 2: > poll_schedule_timeout+0x96/0x160 > do_sys_poll+0x966/0xb30 > __se_sys_ppoll+0x1c3/0x210 > __x64_sys_ppoll+0x71/0x90 > x64_sys_call+0x3079/0x32b0 > do_syscall_64+0xfa/0x3b0 > entry_SYSCALL_64_after_hwframe+0x77/0x7f > > value changed: 0x00000000 -> 0x00000001 > > or concurrent write (example taken from > https://syzkaller.appspot.com/bug?extid=4c7af974f816af4ede2a): > > BUG: KCSAN: data-race in pollwake / pollwake > > write to 0xffffc90000e539e0 of 4 bytes by task 3308 on cpu 1: > __pollwake fs/select.c:195 [inline] > pollwake+0xb6/0x100 fs/select.c:215 > __wake_up_common kernel/sched/wait.c:89 [inline] > __wake_up_common_lock kernel/sched/wait.c:106 [inline] > __wake_up_sync_key+0x4f/0x80 kernel/sched/wait.c:173 > anon_pipe_write+0x8ba/0xaa0 fs/pipe.c:594 > new_sync_write fs/read_write.c:593 [inline] > vfs_write+0x4a0/0x8e0 fs/read_write.c:686 > ksys_write+0xda/0x1a0 fs/read_write.c:738 > __do_sys_write fs/read_write.c:749 [inline] > __se_sys_write fs/read_write.c:746 [inline] > __x64_sys_write+0x40/0x50 fs/read_write.c:746 > x64_sys_call+0x2cdd/0x2fb0 arch/x86/include/generated/asm/syscalls_64.h:2 > do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline] > do_syscall_64+0xd2/0x200 arch/x86/entry/syscall_64.c:94 > entry_SYSCALL_64_after_hwframe+0x77/0x7f > > write to 0xffffc90000e539e0 of 4 bytes by task 4163 on cpu 0: > __pollwake fs/select.c:195 [inline] > pollwake+0xb6/0x100 fs/select.c:215 > __wake_up_common kernel/sched/wait.c:89 [inline] > __wake_up_common_lock kernel/sched/wait.c:106 [inline] > __wake_up_sync_key+0x4f/0x80 kernel/sched/wait.c:173 > anon_pipe_write+0x8ba/0xaa0 fs/pipe.c:594 > new_sync_write fs/read_write.c:593 [inline] > vfs_write+0x4a0/0x8e0 fs/read_write.c:686 > ksys_write+0xda/0x1a0 fs/read_write.c:738 > __do_sys_write fs/read_write.c:749 [inline] > __se_sys_write fs/read_write.c:746 [inline] > __x64_sys_write+0x40/0x50 fs/read_write.c:746 > x64_sys_call+0x2cdd/0x2fb0 arch/x86/include/generated/asm/syscalls_64.h:2 > do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline] > do_syscall_64+0xd2/0x200 arch/x86/entry/syscall_64.c:94 > entry_SYSCALL_64_after_hwframe+0x77/0x7f > > value changed: 0x00000000 -> 0x00000001 > > Using _ONCE() seems makes KCSAN quiet, i.e.: > > diff --git a/fs/select.c b/fs/select.c > index 9fb650d03d52..082cf60c7e23 100644 > --- a/fs/select.c > +++ b/fs/select.c > @@ -192,7 +192,7 @@ static int __pollwake(wait_queue_entry_t *wait, unsigned mode, int sync, void *k > * and is paired with smp_store_mb() in poll_schedule_timeout. > */ > smp_wmb(); > - pwq->triggered = 1; > + WRITE_ONCE(pwq->triggered, 1); > > /* > * Perform the default wake up operation using a dummy > @@ -237,7 +237,7 @@ static int poll_schedule_timeout(struct poll_wqueues *pwq, int state, > int rc = -EINTR; > > set_current_state(state); > - if (!pwq->triggered) > + if (!READ_ONCE(pwq->triggered)) > rc = schedule_hrtimeout_range(expires, slack, HRTIMER_MODE_ABS); > __set_current_state(TASK_RUNNING); > > but I'm curious whether this is a real fix for the real bug or > KCSAN is just unable to handle smp_wmb()/smp_store_mb() trick. So KCSAN is really trigger-happy about issues like this. There's no practical issue here because it is hard to imagine how the compiler could compile the above code using some intermediate values stored into 'triggered' or multiple fetches from 'triggered'. But for the cleanliness of code and silencing of KCSAN your changes make sense. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
