* Re: [syzbot] WARNING: locking bug in umh_complete [not found] <20230127014137.4906-1-hdanton@sina.com> @ 2023-02-03 10:22 ` Tetsuo Handa 2023-02-03 10:48 ` Tetsuo Handa 2023-02-03 12:00 ` [syzbot] WARNING: locking bug in umh_complete Peter Zijlstra 0 siblings, 2 replies; 19+ messages in thread From: Tetsuo Handa @ 2023-02-03 10:22 UTC (permalink / raw) To: Hillf Danton, Peter Zijlstra (Intel), Ingo Molnar, Rafael J. Wysocki Cc: linux-kernel, mcgrof, Linus Torvalds, syzkaller-bugs, syzbot On 2023/01/27 10:41, Hillf Danton wrote: > On Thu, 26 Jan 2023 14:27:51 -0800 >> syzbot found the following issue on: >> >> HEAD commit: 71ab9c3e2253 net: fix UaF in netns ops registration error .. >> git tree: net >> console output: https://syzkaller.appspot.com/x/log.txt?x=10f86a56480000 >> kernel config: https://syzkaller.appspot.com/x/.config?x=899d86a7610a0ea0 >> dashboard link: https://syzkaller.appspot.com/bug?extid=6cd18e123583550cf469 >> compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2 >> >> Unfortunately, I don't have any reproducer for this issue yet. >> >> Downloadable assets: >> disk image: https://storage.googleapis.com/syzbot-assets/54c953096fdb/disk-71ab9c3e.raw.xz >> vmlinux: https://storage.googleapis.com/syzbot-assets/644d72265810/vmlinux-71ab9c3e.xz >> kernel image: https://storage.googleapis.com/syzbot-assets/16e994579ca5/bzImage-71ab9c3e.xz >> >> IMPORTANT: if you fix the issue, please add the following tag to the commit: >> Reported-by: syzbot+6cd18e123583550cf469@syzkaller.appspotmail.com >> >> ------------[ cut here ]------------ >> DEBUG_LOCKS_WARN_ON(1) >> WARNING: CPU: 0 PID: 46 at kernel/locking/lockdep.c:231 hlock_class kernel/locking/lockdep.c:231 [inline] >> WARNING: CPU: 0 PID: 46 at kernel/locking/lockdep.c:231 hlock_class kernel/locking/lockdep.c:220 [inline] >> WARNING: CPU: 0 PID: 46 at kernel/locking/lockdep.c:231 check_wait_context kernel/locking/lockdep.c:4729 [inline] >> WARNING: CPU: 0 PID: 46 at kernel/locking/lockdep.c:231 __lock_acquire+0x13ae/0x56d0 kernel/locking/lockdep.c:5005 >> Modules linked in: >> CPU: 0 PID: 46 Comm: kworker/u4:3 Not tainted 6.2.0-rc4-syzkaller-00191-g71ab9c3e2253 #0 >> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/12/2023 >> Workqueue: events_unbound call_usermodehelper_exec_work >> RIP: 0010:hlock_class kernel/locking/lockdep.c:231 [inline] >> RIP: 0010:hlock_class kernel/locking/lockdep.c:220 [inline] >> RIP: 0010:check_wait_context kernel/locking/lockdep.c:4729 [inline] >> RIP: 0010:__lock_acquire+0x13ae/0x56d0 kernel/locking/lockdep.c:5005 >> Code: 08 84 d2 0f 85 56 41 00 00 8b 15 55 7b 0f 0d 85 d2 0f 85 d5 fd ff ff 48 c7 c6 c0 51 4c 8a 48 c7 c7 20 4b 4c 8a e8 92 e1 5b 08 <0f> 0b 31 ed e9 50 f0 ff ff 48 c7 c0 a8 2b 73 8e 48 ba 00 00 00 00 >> RSP: 0018:ffffc90000b77a30 EFLAGS: 00010082 >> RAX: 0000000000000000 RBX: ffff88801272a778 RCX: 0000000000000000 >> RDX: ffff888012729d40 RSI: ffffffff8166822c RDI: fffff5200016ef38 >> RBP: 0000000000001b2c R08: 0000000000000005 R09: 0000000000000000 >> R10: 0000000080000001 R11: 0000000000000001 R12: ffff88801272a7c8 >> R13: ffff888012729d40 R14: ffffc9000397fb28 R15: 0000000000040000 >> FS: 0000000000000000(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000 >> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >> CR2: 00007fa5a95e81d0 CR3: 00000000284d2000 CR4: 00000000003506f0 >> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 >> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 >> Call Trace: >> <TASK> >> lock_acquire kernel/locking/lockdep.c:5668 [inline] >> lock_acquire+0x1e3/0x630 kernel/locking/lockdep.c:5633 >> __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline] >> _raw_spin_lock_irqsave+0x3d/0x60 kernel/locking/spinlock.c:162 >> complete+0x1d/0x1f0 kernel/sched/completion.c:32 >> umh_complete+0x32/0x90 kernel/umh.c:59 >> call_usermodehelper_exec_sync kernel/umh.c:144 [inline] >> call_usermodehelper_exec_work+0x115/0x180 kernel/umh.c:167 >> process_one_work+0x9bf/0x1710 kernel/workqueue.c:2289 >> worker_thread+0x669/0x1090 kernel/workqueue.c:2436 >> kthread+0x2e8/0x3a0 kernel/kthread.c:376 >> ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308 >> </TASK> > > This is an interesting case - given done initialized on stack, no garbage > should have been detected by lockdep. > > One explanation to the report is uaf on the waker side, and it can be > tested with the diff below when a reproducer is available. > > Hillf > > --- a/kernel/umh.c > +++ b/kernel/umh.c > @@ -452,6 +452,12 @@ int call_usermodehelper_exec(struct subp > /* umh_complete() will see NULL and free sub_info */ > if (xchg(&sub_info->complete, NULL)) > goto unlock; > + else { > + /* wait for umh_complete() to finish in a bid to avoid > + * uaf because done is destructed > + */ > + wait_for_completion(&done); > + } > } > > wait_done: > -- Yes, this bug is caused by commit f5d39b020809 ("freezer,sched: Rewrite core freezer logic"), for that commit for unknown reason omits wait_for_completion(&done) call when wait_for_completion_state(&done, state) returned -ERESTARTSYS. Peter, is it safe to restore wait_for_completion(&done) call? ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [syzbot] WARNING: locking bug in umh_complete 2023-02-03 10:22 ` [syzbot] WARNING: locking bug in umh_complete Tetsuo Handa @ 2023-02-03 10:48 ` Tetsuo Handa 2023-02-03 12:19 ` Peter Zijlstra 2023-02-03 12:00 ` [syzbot] WARNING: locking bug in umh_complete Peter Zijlstra 1 sibling, 1 reply; 19+ messages in thread From: Tetsuo Handa @ 2023-02-03 10:48 UTC (permalink / raw) To: Peter Zijlstra (Intel), Ingo Molnar, Rafael J. Wysocki Cc: linux-kernel, mcgrof, Linus Torvalds, syzkaller-bugs, syzbot, Hillf Danton On 2023/02/03 19:22, Tetsuo Handa wrote: > Yes, this bug is caused by commit f5d39b020809 ("freezer,sched: Rewrite core freezer > logic"), for that commit for unknown reason omits wait_for_completion(&done) call > when wait_for_completion_state(&done, state) returned -ERESTARTSYS. > > Peter, is it safe to restore wait_for_completion(&done) call? > Something like this? diff --git a/kernel/umh.c b/kernel/umh.c index 850631518665..97230edb1849 100644 --- a/kernel/umh.c +++ b/kernel/umh.c @@ -441,8 +441,8 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait) if (wait & UMH_KILLABLE) state |= TASK_KILLABLE; - if (wait & UMH_FREEZABLE) - state |= TASK_FREEZABLE; + //if (wait & UMH_FREEZABLE) + // state |= TASK_FREEZABLE; retval = wait_for_completion_state(&done, state); if (!retval) @@ -452,7 +452,9 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait) /* umh_complete() will see NULL and free sub_info */ if (xchg(&sub_info->complete, NULL)) goto unlock; + /* fallthrough, umh_complete() was already called */ } + wait_for_completion(&done); wait_done: retval = sub_info->retval; How does TASK_FREEZABLE affect here? Since call_usermodehelper_exec() is a function for starting and waiting for termination of a userspace process (which is subjected to freezing), the caller of call_usermodehelper_exec() can't wait for the termination of that userspace process if that process was frozen, and wait_for_completion() blocks forever? ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [syzbot] WARNING: locking bug in umh_complete 2023-02-03 10:48 ` Tetsuo Handa @ 2023-02-03 12:19 ` Peter Zijlstra 2023-02-03 12:30 ` Peter Zijlstra 0 siblings, 1 reply; 19+ messages in thread From: Peter Zijlstra @ 2023-02-03 12:19 UTC (permalink / raw) To: Tetsuo Handa Cc: Ingo Molnar, Rafael J. Wysocki, linux-kernel, mcgrof, Linus Torvalds, syzkaller-bugs, syzbot, Hillf Danton On Fri, Feb 03, 2023 at 07:48:35PM +0900, Tetsuo Handa wrote: > On 2023/02/03 19:22, Tetsuo Handa wrote: > > Yes, this bug is caused by commit f5d39b020809 ("freezer,sched: Rewrite core freezer > > logic"), for that commit for unknown reason omits wait_for_completion(&done) call > > when wait_for_completion_state(&done, state) returned -ERESTARTSYS. > > > > Peter, is it safe to restore wait_for_completion(&done) call? > > > > Something like this? > > diff --git a/kernel/umh.c b/kernel/umh.c > index 850631518665..97230edb1849 100644 > --- a/kernel/umh.c > +++ b/kernel/umh.c > @@ -441,8 +441,8 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait) > if (wait & UMH_KILLABLE) > state |= TASK_KILLABLE; > > - if (wait & UMH_FREEZABLE) > - state |= TASK_FREEZABLE; > + //if (wait & UMH_FREEZABLE) > + // state |= TASK_FREEZABLE; > > retval = wait_for_completion_state(&done, state); > if (!retval) > @@ -452,7 +452,9 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait) > /* umh_complete() will see NULL and free sub_info */ > if (xchg(&sub_info->complete, NULL)) > goto unlock; > + /* fallthrough, umh_complete() was already called */ > } > + wait_for_completion(&done); > > wait_done: > retval = sub_info->retval; > > How does TASK_FREEZABLE affect here? It marks those waits that are safe to convert to a frozen state. > Since call_usermodehelper_exec() is a function for starting and > waiting for termination of a userspace process (which is subjected to > freezing), the caller of call_usermodehelper_exec() can't wait for the > termination of that userspace process if that process was frozen, and > wait_for_completion() > blocks forever? It'll probably make the freeze fail and abort the suspend. We first freezer userspace (including the helper), then we try and freeze all the kernel threads. If we can't, we error out and abort -- waking everything back up. But now I realize what I missed before, wait_for_completion() it not interruptible. I think the right fix is to: state &= ~TASK_KILLABLE; wait_for_completion_state(&done, state); Also, put in a comment.. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [syzbot] WARNING: locking bug in umh_complete 2023-02-03 12:19 ` Peter Zijlstra @ 2023-02-03 12:30 ` Peter Zijlstra 2023-02-03 12:59 ` Tetsuo Handa 0 siblings, 1 reply; 19+ messages in thread From: Peter Zijlstra @ 2023-02-03 12:30 UTC (permalink / raw) To: Tetsuo Handa Cc: Ingo Molnar, Rafael J. Wysocki, linux-kernel, mcgrof, Linus Torvalds, syzkaller-bugs, syzbot, Hillf Danton On Fri, Feb 03, 2023 at 01:19:53PM +0100, Peter Zijlstra wrote: > On Fri, Feb 03, 2023 at 07:48:35PM +0900, Tetsuo Handa wrote: > > On 2023/02/03 19:22, Tetsuo Handa wrote: > > > Yes, this bug is caused by commit f5d39b020809 ("freezer,sched: Rewrite core freezer > > > logic"), for that commit for unknown reason omits wait_for_completion(&done) call > > > when wait_for_completion_state(&done, state) returned -ERESTARTSYS. > > > > > > Peter, is it safe to restore wait_for_completion(&done) call? > > > > > > > Something like this? > > > > diff --git a/kernel/umh.c b/kernel/umh.c > > index 850631518665..97230edb1849 100644 > > --- a/kernel/umh.c > > +++ b/kernel/umh.c > > @@ -441,8 +441,8 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait) > > if (wait & UMH_KILLABLE) > > state |= TASK_KILLABLE; > > > > - if (wait & UMH_FREEZABLE) > > - state |= TASK_FREEZABLE; > > + //if (wait & UMH_FREEZABLE) > > + // state |= TASK_FREEZABLE; > > > > retval = wait_for_completion_state(&done, state); > > if (!retval) > > @@ -452,7 +452,9 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait) > > /* umh_complete() will see NULL and free sub_info */ > > if (xchg(&sub_info->complete, NULL)) > > goto unlock; > > + /* fallthrough, umh_complete() was already called */ > > } > > + wait_for_completion(&done); > > > > wait_done: > > retval = sub_info->retval; > > > > How does TASK_FREEZABLE affect here? > > It marks those waits that are safe to convert to a frozen state. > > > Since call_usermodehelper_exec() is a function for starting and > > waiting for termination of a userspace process (which is subjected to > > freezing), the caller of call_usermodehelper_exec() can't wait for the > > termination of that userspace process if that process was frozen, and > > wait_for_completion() > > blocks forever? > > It'll probably make the freeze fail and abort the suspend. We first > freezer userspace (including the helper), then we try and freeze all the > kernel threads. If we can't, we error out and abort -- waking everything > back up. > > But now I realize what I missed before, wait_for_completion() it not > interruptible. > > I think the right fix is to: > > state &= ~TASK_KILLABLE; state &= ~__TASK_WAKEKILL; we don't want to mask out UNINTERUPTIBLE, that would be bad. > wait_for_completion_state(&done, state); > > Also, put in a comment.. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [syzbot] WARNING: locking bug in umh_complete 2023-02-03 12:30 ` Peter Zijlstra @ 2023-02-03 12:59 ` Tetsuo Handa 2023-02-03 14:31 ` Peter Zijlstra 0 siblings, 1 reply; 19+ messages in thread From: Tetsuo Handa @ 2023-02-03 12:59 UTC (permalink / raw) To: Peter Zijlstra Cc: Ingo Molnar, Rafael J. Wysocki, linux-kernel, mcgrof, Linus Torvalds, syzkaller-bugs, syzbot, Hillf Danton On 2023/02/03 21:30, Peter Zijlstra wrote: >> I think the right fix is to: >> >> state &= ~TASK_KILLABLE; > > state &= ~__TASK_WAKEKILL; > > we don't want to mask out UNINTERUPTIBLE, that would be bad. This code was made killable as a solution for CVE-2012-4398. Although OOM reaper is available today, making back to unkillable is not smart. > >> wait_for_completion_state(&done, state); >> >> Also, put in a comment.. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [syzbot] WARNING: locking bug in umh_complete 2023-02-03 12:59 ` Tetsuo Handa @ 2023-02-03 14:31 ` Peter Zijlstra 2023-02-04 0:48 ` Tetsuo Handa 2023-02-15 10:33 ` [tip: sched/urgent] freezer,umh: Fix call_usermode_helper_exec() vs SIGKILL tip-bot2 for Peter Zijlstra 0 siblings, 2 replies; 19+ messages in thread From: Peter Zijlstra @ 2023-02-03 14:31 UTC (permalink / raw) To: Tetsuo Handa Cc: Ingo Molnar, Rafael J. Wysocki, linux-kernel, mcgrof, Linus Torvalds, syzkaller-bugs, syzbot, Hillf Danton On Fri, Feb 03, 2023 at 09:59:51PM +0900, Tetsuo Handa wrote: > On 2023/02/03 21:30, Peter Zijlstra wrote: > >> I think the right fix is to: > >> > >> state &= ~TASK_KILLABLE; > > > > state &= ~__TASK_WAKEKILL; > > > > we don't want to mask out UNINTERUPTIBLE, that would be bad. > > This code was made killable as a solution for CVE-2012-4398. > Although OOM reaper is available today, making back to unkillable is not smart. Yes, I meant something like so. diff --git a/kernel/umh.c b/kernel/umh.c index 850631518665..0e69e1e4b6fe 100644 --- a/kernel/umh.c +++ b/kernel/umh.c @@ -438,21 +438,24 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait) if (wait == UMH_NO_WAIT) /* task has freed sub_info */ goto unlock; - if (wait & UMH_KILLABLE) - state |= TASK_KILLABLE; - - if (wait & UMH_FREEZABLE) + if (wait & UMH_FREEZABLE) { state |= TASK_FREEZABLE; - retval = wait_for_completion_state(&done, state); - if (!retval) - goto wait_done; - if (wait & UMH_KILLABLE) { + retval = wait_for_completion_state(&done, state | TASK_KILLABLE); + if (!retval) + goto wait_done; + /* umh_complete() will see NULL and free sub_info */ if (xchg(&sub_info->complete, NULL)) goto unlock; + + /* + * fallthrough; in case of -ERESTARTSYS now do uninterruptible + * wait_for_completion(). + */ } + wait_for_completion_state(&done, state); wait_done: retval = sub_info->retval; ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [syzbot] WARNING: locking bug in umh_complete 2023-02-03 14:31 ` Peter Zijlstra @ 2023-02-04 0:48 ` Tetsuo Handa 2023-02-06 15:51 ` Luis Chamberlain 2023-02-13 15:34 ` Peter Zijlstra 2023-02-15 10:33 ` [tip: sched/urgent] freezer,umh: Fix call_usermode_helper_exec() vs SIGKILL tip-bot2 for Peter Zijlstra 1 sibling, 2 replies; 19+ messages in thread From: Tetsuo Handa @ 2023-02-04 0:48 UTC (permalink / raw) To: Peter Zijlstra Cc: Ingo Molnar, Rafael J. Wysocki, linux-kernel, mcgrof, Linus Torvalds, syzkaller-bugs, syzbot, Hillf Danton On 2023/02/03 23:31, Peter Zijlstra wrote: > Yes, I meant something like so. > > > diff --git a/kernel/umh.c b/kernel/umh.c > index 850631518665..0e69e1e4b6fe 100644 > --- a/kernel/umh.c > +++ b/kernel/umh.c > @@ -438,21 +438,24 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait) > if (wait == UMH_NO_WAIT) /* task has freed sub_info */ > goto unlock; > > - if (wait & UMH_KILLABLE) > - state |= TASK_KILLABLE; > - > - if (wait & UMH_FREEZABLE) > + if (wait & UMH_FREEZABLE) { > state |= TASK_FREEZABLE; > > - retval = wait_for_completion_state(&done, state); > - if (!retval) > - goto wait_done; > - > if (wait & UMH_KILLABLE) { > + retval = wait_for_completion_state(&done, state | TASK_KILLABLE); > + if (!retval) > + goto wait_done; > + > /* umh_complete() will see NULL and free sub_info */ > if (xchg(&sub_info->complete, NULL)) > goto unlock; > + > + /* > + * fallthrough; in case of -ERESTARTSYS now do uninterruptible > + * wait_for_completion(). > + */ > } > + wait_for_completion_state(&done, state); > > wait_done: > retval = sub_info->retval; Please fold below diff, provided that wait_for_completion_state(TASK_FREEZABLE) does not return when the current thread was frozen. (If wait_for_completion_state(TASK_FREEZABLE) returns when the current thread was frozen, we will fail to execute the retval = sub_info->retval; line in order to get the exit code after the current thread is thawed...) diff --git a/kernel/umh.c b/kernel/umh.c index 0e69e1e4b6fe..a776920ec051 100644 --- a/kernel/umh.c +++ b/kernel/umh.c @@ -431,35 +431,38 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait) * This makes it possible to use umh_complete to free * the data structure in case of UMH_NO_WAIT. */ sub_info->complete = (wait == UMH_NO_WAIT) ? NULL : &done; sub_info->wait = wait; queue_work(system_unbound_wq, &sub_info->work); if (wait == UMH_NO_WAIT) /* task has freed sub_info */ goto unlock; - if (wait & UMH_FREEZABLE) { + if (wait & UMH_FREEZABLE) state |= TASK_FREEZABLE; if (wait & UMH_KILLABLE) { retval = wait_for_completion_state(&done, state | TASK_KILLABLE); if (!retval) goto wait_done; /* umh_complete() will see NULL and free sub_info */ if (xchg(&sub_info->complete, NULL)) goto unlock; /* * fallthrough; in case of -ERESTARTSYS now do uninterruptible - * wait_for_completion(). + * wait_for_completion_state(). Since umh_complete() shall call + * complete() in a moment if xchg() above returned NULL, this + * uninterruptible wait_for_completion_state() will not block + * SIGKILL'ed process for so long. */ } wait_for_completion_state(&done, state); wait_done: retval = sub_info->retval; out: call_usermodehelper_freeinfo(sub_info); unlock: helper_unlock(); ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [syzbot] WARNING: locking bug in umh_complete 2023-02-04 0:48 ` Tetsuo Handa @ 2023-02-06 15:51 ` Luis Chamberlain 2023-02-13 15:14 ` Peter Zijlstra 2023-02-13 15:27 ` Peter Zijlstra 2023-02-13 15:34 ` Peter Zijlstra 1 sibling, 2 replies; 19+ messages in thread From: Luis Chamberlain @ 2023-02-06 15:51 UTC (permalink / raw) To: Tetsuo Handa, Schspa Shi Cc: Peter Zijlstra, Ingo Molnar, Rafael J. Wysocki, linux-kernel, Linus Torvalds, syzkaller-bugs, syzbot, Hillf Danton On Sat, Feb 04, 2023 at 09:48:39AM +0900, Tetsuo Handa wrote: > On 2023/02/03 23:31, Peter Zijlstra wrote: > > Yes, I meant something like so. > > > > > > diff --git a/kernel/umh.c b/kernel/umh.c > > index 850631518665..0e69e1e4b6fe 100644 > > --- a/kernel/umh.c > > +++ b/kernel/umh.c > > @@ -438,21 +438,24 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait) > > if (wait == UMH_NO_WAIT) /* task has freed sub_info */ > > goto unlock; > > > > - if (wait & UMH_KILLABLE) > > - state |= TASK_KILLABLE; > > - > > - if (wait & UMH_FREEZABLE) > > + if (wait & UMH_FREEZABLE) { > > state |= TASK_FREEZABLE; > > > > - retval = wait_for_completion_state(&done, state); > > - if (!retval) > > - goto wait_done; > > - > > if (wait & UMH_KILLABLE) { > > + retval = wait_for_completion_state(&done, state | TASK_KILLABLE); > > + if (!retval) > > + goto wait_done; > > + > > /* umh_complete() will see NULL and free sub_info */ > > if (xchg(&sub_info->complete, NULL)) > > goto unlock; > > + > > + /* > > + * fallthrough; in case of -ERESTARTSYS now do uninterruptible > > + * wait_for_completion(). > > + */ > > } > > + wait_for_completion_state(&done, state); > > > > wait_done: > > retval = sub_info->retval; > > Please fold below diff, provided that wait_for_completion_state(TASK_FREEZABLE) > does not return when the current thread was frozen. (If > wait_for_completion_state(TASK_FREEZABLE) returns when the current thread was > frozen, we will fail to execute the > > retval = sub_info->retval; > > line in order to get the exit code after the current thread is thawed...) > > diff --git a/kernel/umh.c b/kernel/umh.c > index 0e69e1e4b6fe..a776920ec051 100644 > --- a/kernel/umh.c > +++ b/kernel/umh.c > @@ -431,35 +431,38 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait) > * This makes it possible to use umh_complete to free > * the data structure in case of UMH_NO_WAIT. > */ > sub_info->complete = (wait == UMH_NO_WAIT) ? NULL : &done; > sub_info->wait = wait; > > queue_work(system_unbound_wq, &sub_info->work); > if (wait == UMH_NO_WAIT) /* task has freed sub_info */ > goto unlock; > > - if (wait & UMH_FREEZABLE) { > + if (wait & UMH_FREEZABLE) > state |= TASK_FREEZABLE; > > if (wait & UMH_KILLABLE) { > retval = wait_for_completion_state(&done, state | TASK_KILLABLE); > if (!retval) > goto wait_done; > > /* umh_complete() will see NULL and free sub_info */ > if (xchg(&sub_info->complete, NULL)) > goto unlock; > > /* > * fallthrough; in case of -ERESTARTSYS now do uninterruptible > - * wait_for_completion(). > + * wait_for_completion_state(). Since umh_complete() shall call > + * complete() in a moment if xchg() above returned NULL, this > + * uninterruptible wait_for_completion_state() will not block > + * SIGKILL'ed process for so long. > */ > } > wait_for_completion_state(&done, state); I think this seems to be the same issue that Schspa Shi reported / provided a fix sugggestion for [0]. This lead me to ask if: a) incorrect usage of completion on stack could be generic and; b) if we should instead have an API helper for that? Although he already implemented a suggestion for b) to answer a) we need some SmPL constructs yet to be written by Schspa. The reason I asked for b) is that if this is a regular pattern it begs for a) as this sort of issue could be prevalent in other places. So the status of Schspa's work was that he was going to work on the SmPL grammar to check how frequent this incorrect patern could be found. Please let me know your thoughts as perhaps this issue / discussion didn't get on Peter's radar as it was rececently discussed with Schspa despite being on Cc. [0] https://lore.kernel.org/all/m2pmcoag55.fsf@gmail.com/T/#u Luis ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [syzbot] WARNING: locking bug in umh_complete 2023-02-06 15:51 ` Luis Chamberlain @ 2023-02-13 15:14 ` Peter Zijlstra 2023-02-13 15:27 ` Peter Zijlstra 1 sibling, 0 replies; 19+ messages in thread From: Peter Zijlstra @ 2023-02-13 15:14 UTC (permalink / raw) To: Luis Chamberlain Cc: Tetsuo Handa, Schspa Shi, Ingo Molnar, Rafael J. Wysocki, linux-kernel, Linus Torvalds, syzkaller-bugs, syzbot, Hillf Danton On Mon, Feb 06, 2023 at 07:51:16AM -0800, Luis Chamberlain wrote: > I think this seems to be the same issue that Schspa Shi reported / provided a > fix sugggestion for [0]. This lead me to ask if: > > a) incorrect usage of completion on stack could be generic and; > b) if we should instead have an API helper for that? > > Although he already implemented a suggestion for b) to answer a) we need > some SmPL constructs yet to be written by Schspa. The reason I asked for > b) is that if this is a regular pattern it begs for a) as this sort of > issue could be prevalent in other places. So the status of Schspa's work > was that he was going to work on the SmPL grammar to check how frequent > this incorrect patern could be found. > > Please let me know your thoughts as perhaps this issue / discussion > didn't get on Peter's radar as it was rececently discussed with Schspa > despite being on Cc. > > [0] https://lore.kernel.org/all/m2pmcoag55.fsf@gmail.com/T/#u -ETOODAMNMUCHEMAIL :-/ Urgh, esp. : https://lore.kernel.org/all/m2pmcoag55.fsf@gmail.com/T/#m9f0105d28fcfe4947a2583cd3d425169c4fe5dfa is quite insane. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [syzbot] WARNING: locking bug in umh_complete 2023-02-06 15:51 ` Luis Chamberlain 2023-02-13 15:14 ` Peter Zijlstra @ 2023-02-13 15:27 ` Peter Zijlstra 2023-02-14 2:31 ` Schspa Shi 1 sibling, 1 reply; 19+ messages in thread From: Peter Zijlstra @ 2023-02-13 15:27 UTC (permalink / raw) To: Luis Chamberlain Cc: Tetsuo Handa, Schspa Shi, Ingo Molnar, Rafael J. Wysocki, linux-kernel, Linus Torvalds, syzkaller-bugs, syzbot, Hillf Danton On Mon, Feb 06, 2023 at 07:51:16AM -0800, Luis Chamberlain wrote: > I think this seems to be the same issue that Schspa Shi reported / provided a > fix sugggestion for [0]. This lead me to ask if: > > a) incorrect usage of completion on stack could be generic and; > b) if we should instead have an API helper for that? > > Although he already implemented a suggestion for b) to answer a) we need > some SmPL constructs yet to be written by Schspa. The reason I asked for > b) is that if this is a regular pattern it begs for a) as this sort of > issue could be prevalent in other places. So the status of Schspa's work > was that he was going to work on the SmPL grammar to check how frequent > this incorrect patern could be found. Do I read correctly, from you above alphabet-soup, that someone is working on some static analysis for on-stack completions or something? If so, perhaps the simplest rule would to be ensure there is an unconditional uninterruptible wait-for-completion() before going out of scope. This latter can be spelled like wait_for_completion() or wait_for_completion_state(TASK_UNINTERRUPTIBLE). More specifically, TASK_INTERRUPTIBLE and TASK_WAKEKILL must not be set in the state mask for the wait to be uninterruptible. If it cannot be proven, raise a warning and audit or somesuch. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [syzbot] WARNING: locking bug in umh_complete 2023-02-13 15:27 ` Peter Zijlstra @ 2023-02-14 2:31 ` Schspa Shi 2023-02-21 21:54 ` Luis Chamberlain 2023-02-22 9:34 ` Peter Zijlstra 0 siblings, 2 replies; 19+ messages in thread From: Schspa Shi @ 2023-02-14 2:31 UTC (permalink / raw) To: Peter Zijlstra Cc: Luis Chamberlain, Tetsuo Handa, Ingo Molnar, Rafael J. Wysocki, linux-kernel, Linus Torvalds, syzkaller-bugs, syzbot, Hillf Danton Peter Zijlstra <peterz@infradead.org> writes: > On Mon, Feb 06, 2023 at 07:51:16AM -0800, Luis Chamberlain wrote: > >> I think this seems to be the same issue that Schspa Shi reported / provided a >> fix sugggestion for [0]. This lead me to ask if: >> >> a) incorrect usage of completion on stack could be generic and; >> b) if we should instead have an API helper for that? >> >> Although he already implemented a suggestion for b) to answer a) we need >> some SmPL constructs yet to be written by Schspa. The reason I asked for >> b) is that if this is a regular pattern it begs for a) as this sort of >> issue could be prevalent in other places. So the status of Schspa's work >> was that he was going to work on the SmPL grammar to check how frequent >> this incorrect patern could be found. > > Do I read correctly, from you above alphabet-soup, that someone is > working on some static analysis for on-stack completions or something? > Yes, I was trying to do this. > If so, perhaps the simplest rule would to be ensure there is an > unconditional uninterruptible wait-for-completion() before going out of > scope. > > This latter can be spelled like wait_for_completion() or > wait_for_completion_state(TASK_UNINTERRUPTIBLE). More specifically, > TASK_INTERRUPTIBLE and TASK_WAKEKILL must not be set in the state mask > for the wait to be uninterruptible. > > If it cannot be proven, raise a warning and audit or somesuch. This is a good suggestion. I have written a SmPL patch to complete this check, and now I need to rule out the situation that the driver has added an additional lock to protect it. And I have found a lot of bad usage, should we consider adding a new helper API to simplify the fix this? Such as: + +void complete_on_stack(struct completion **x) +{ + struct completion *comp = xchg(*x, NULL); + + if (comp) + complete(comp); +} +EXPORT_SYMBOL(complete_on_stack); + +int __sched wait_for_completion_state_on_stack(struct completion **x, + unsigned int state) +{ + struct completion *comp = *x; + int retval; + + retval = wait_for_completion_state(comp, state); + if (retval) { + if (xchg(*x, NULL)) + return retval; + + /* + * complete_on_stack will call complete shortly. + */ + wait_for_completion(comp); + } + + return retval; +} +EXPORT_SYMBOL(wait_for_completion_state_on_stack); Link: https://lore.kernel.org/all/20221115140233.21981-1-schspa@gmail.com/T/#mf6a41a7009bb47af1b15adf2b7b355e495f609c4 -- BRs Schspa Shi ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [syzbot] WARNING: locking bug in umh_complete 2023-02-14 2:31 ` Schspa Shi @ 2023-02-21 21:54 ` Luis Chamberlain 2023-02-22 9:34 ` Peter Zijlstra 1 sibling, 0 replies; 19+ messages in thread From: Luis Chamberlain @ 2023-02-21 21:54 UTC (permalink / raw) To: Schspa Shi Cc: Peter Zijlstra, Tetsuo Handa, Ingo Molnar, Rafael J. Wysocki, linux-kernel, Linus Torvalds, syzkaller-bugs, syzbot, Hillf Danton On Tue, Feb 14, 2023 at 10:31:58AM +0800, Schspa Shi wrote: > I have written a SmPL patch to complete this > check <-- etc --> > And I have found a lot of bad usage Can you share what you found here? Just a simple list of affected files. Luis ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [syzbot] WARNING: locking bug in umh_complete 2023-02-14 2:31 ` Schspa Shi 2023-02-21 21:54 ` Luis Chamberlain @ 2023-02-22 9:34 ` Peter Zijlstra 2023-02-27 7:57 ` Schspa Shi 1 sibling, 1 reply; 19+ messages in thread From: Peter Zijlstra @ 2023-02-22 9:34 UTC (permalink / raw) To: Schspa Shi Cc: Luis Chamberlain, Tetsuo Handa, Ingo Molnar, Rafael J. Wysocki, linux-kernel, Linus Torvalds, syzkaller-bugs, syzbot, Hillf Danton On Tue, Feb 14, 2023 at 10:31:58AM +0800, Schspa Shi wrote: > Peter Zijlstra <peterz@infradead.org> writes: > > If so, perhaps the simplest rule would to be ensure there is an > > unconditional uninterruptible wait-for-completion() before going out of > > scope. > > > > This latter can be spelled like wait_for_completion() or > > wait_for_completion_state(TASK_UNINTERRUPTIBLE). More specifically, > > TASK_INTERRUPTIBLE and TASK_WAKEKILL must not be set in the state mask > > for the wait to be uninterruptible. > > > > If it cannot be proven, raise a warning and audit or somesuch. > > This is a good suggestion. I have written a SmPL patch to complete this > check, and now I need to rule out the situation that the driver has > added an additional lock to protect it. > > And I have found a lot of bad usage, should we consider adding a new > helper API to simplify the fix this? Please first share some of the locations where this would be applied. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [syzbot] WARNING: locking bug in umh_complete 2023-02-22 9:34 ` Peter Zijlstra @ 2023-02-27 7:57 ` Schspa Shi 0 siblings, 0 replies; 19+ messages in thread From: Schspa Shi @ 2023-02-27 7:57 UTC (permalink / raw) To: Peter Zijlstra Cc: Luis Chamberlain, Tetsuo Handa, Ingo Molnar, Rafael J. Wysocki, linux-kernel, Linus Torvalds, syzkaller-bugs, syzbot, Hillf Danton Peter Zijlstra <peterz@infradead.org> writes: > On Tue, Feb 14, 2023 at 10:31:58AM +0800, Schspa Shi wrote: >> Peter Zijlstra <peterz@infradead.org> writes: > >> > If so, perhaps the simplest rule would to be ensure there is an >> > unconditional uninterruptible wait-for-completion() before going out of >> > scope. >> > >> > This latter can be spelled like wait_for_completion() or >> > wait_for_completion_state(TASK_UNINTERRUPTIBLE). More specifically, >> > TASK_INTERRUPTIBLE and TASK_WAKEKILL must not be set in the state mask >> > for the wait to be uninterruptible. >> > >> > If it cannot be proven, raise a warning and audit or somesuch. >> >> This is a good suggestion. I have written a SmPL patch to complete this >> check, and now I need to rule out the situation that the driver has >> added an additional lock to protect it. >> >> And I have found a lot of bad usage, should we consider adding a new >> helper API to simplify the fix this? > > Please first share some of the locations where this would be applied. Hi Peter: I started a new thread to discuss the SmPL patch. Link: https://lore.kernel.org/all/20230227075346.69658-1-schspa@gmail.com/ -- BRs Schspa Shi ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [syzbot] WARNING: locking bug in umh_complete 2023-02-04 0:48 ` Tetsuo Handa 2023-02-06 15:51 ` Luis Chamberlain @ 2023-02-13 15:34 ` Peter Zijlstra 2023-02-14 11:16 ` Tetsuo Handa 1 sibling, 1 reply; 19+ messages in thread From: Peter Zijlstra @ 2023-02-13 15:34 UTC (permalink / raw) To: Tetsuo Handa Cc: Ingo Molnar, Rafael J. Wysocki, linux-kernel, mcgrof, Linus Torvalds, syzkaller-bugs, syzbot, Hillf Danton On Sat, Feb 04, 2023 at 09:48:39AM +0900, Tetsuo Handa wrote: > Please fold below diff, Find final version below -- typing hard I suppose.. > provided that wait_for_completion_state(TASK_FREEZABLE) > does not return when the current thread was frozen. (If > wait_for_completion_state(TASK_FREEZABLE) returns when the current thread was > frozen, we will fail to execute the FREEZABLE should be transparent; that is it will only return when an actual wakeup was missed, otherwise it will remain asleep. Specifically, the FROZEN thing relies on wait loops to be resillient against spurious wakeups. Consider do_wait_for_common(), the action() := schedule_timeout() might 'suriously' return after thawing, but it will re-validate the actual completion condition and go back to sleep if it hasn't happened yet. OTOH, if the completeion condition has happened (right before the completer itself was frozen for example, but after the waiter was already frozen), then the 'spurious' wakeup on thaw is exactly what was needed, the completion condition is satisfied and the wait terminated. --- Subject: freezer,umh: Fix call_usermode_helper_exec() vs SIGKILL From: Peter Zijlstra <peterz@infradead.org> Date: Fri, 3 Feb 2023 15:31:11 +0100 Tetsuo-San noted that commit f5d39b020809 ("freezer,sched: Rewrite core freezer logic") broke call_usermodehelper_exec() for the KILLABLE case. Specifically it was missed that the second, unconditional, wait_for_completion() was not optional and ensures the on-stack completion is unused before going out-of-scope. Fixes: f5d39b020809 ("freezer,sched: Rewrite core freezer logic") Reported-by: syzbot+6cd18e123583550cf469@syzkaller.appspotmail.com Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Debugged-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Link: https://lkml.kernel.org/r/Y90ar35uKQoUrLEK@hirez.programming.kicks-ass.net --- kernel/umh.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) --- a/kernel/umh.c +++ b/kernel/umh.c @@ -438,21 +438,27 @@ int call_usermodehelper_exec(struct subp if (wait == UMH_NO_WAIT) /* task has freed sub_info */ goto unlock; - if (wait & UMH_KILLABLE) - state |= TASK_KILLABLE; - if (wait & UMH_FREEZABLE) state |= TASK_FREEZABLE; - retval = wait_for_completion_state(&done, state); - if (!retval) - goto wait_done; - if (wait & UMH_KILLABLE) { + retval = wait_for_completion_state(&done, state | TASK_KILLABLE); + if (!retval) + goto wait_done; + /* umh_complete() will see NULL and free sub_info */ if (xchg(&sub_info->complete, NULL)) goto unlock; + + /* + * fallthrough; in case of -ERESTARTSYS now do uninterruptible + * wait_for_completion_state(). Since umh_complete() shall call + * complete() in a moment if xchg() above returned NULL, this + * uninterruptible wait_for_completion_state() will not block + * SIGKILL'ed processes for long. + */ } + wait_for_completion_state(&done, state); wait_done: retval = sub_info->retval; ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [syzbot] WARNING: locking bug in umh_complete 2023-02-13 15:34 ` Peter Zijlstra @ 2023-02-14 11:16 ` Tetsuo Handa 0 siblings, 0 replies; 19+ messages in thread From: Tetsuo Handa @ 2023-02-14 11:16 UTC (permalink / raw) To: Peter Zijlstra Cc: Ingo Molnar, Rafael J. Wysocki, linux-kernel, mcgrof, Linus Torvalds, syzkaller-bugs, syzbot, Hillf Danton On 2023/02/14 0:34, Peter Zijlstra wrote: > On Sat, Feb 04, 2023 at 09:48:39AM +0900, Tetsuo Handa wrote: >> Please fold below diff, > > Find final version below -- typing hard I suppose.. > Thanks for explanation and patch. Please proceed. As per https://lkml.kernel.org/r/20221115140233.21981-1-schspa@gmail.com , I think Schspa Shi <schspa@gmail.com> can be added using Reported-by: tag. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tip: sched/urgent] freezer,umh: Fix call_usermode_helper_exec() vs SIGKILL 2023-02-03 14:31 ` Peter Zijlstra 2023-02-04 0:48 ` Tetsuo Handa @ 2023-02-15 10:33 ` tip-bot2 for Peter Zijlstra 1 sibling, 0 replies; 19+ messages in thread From: tip-bot2 for Peter Zijlstra @ 2023-02-15 10:33 UTC (permalink / raw) To: linux-tip-commits Cc: syzbot+6cd18e123583550cf469, Tetsuo Handa, Peter Zijlstra (Intel), x86, linux-kernel The following commit has been merged into the sched/urgent branch of tip: Commit-ID: eedeb787ebb53de5c5dcf7b7b39d01bf1b0f037d Gitweb: https://git.kernel.org/tip/eedeb787ebb53de5c5dcf7b7b39d01bf1b0f037d Author: Peter Zijlstra <peterz@infradead.org> AuthorDate: Fri, 03 Feb 2023 15:31:11 +01:00 Committer: Peter Zijlstra <peterz@infradead.org> CommitterDate: Mon, 13 Feb 2023 16:36:14 +01:00 freezer,umh: Fix call_usermode_helper_exec() vs SIGKILL Tetsuo-San noted that commit f5d39b020809 ("freezer,sched: Rewrite core freezer logic") broke call_usermodehelper_exec() for the KILLABLE case. Specifically it was missed that the second, unconditional, wait_for_completion() was not optional and ensures the on-stack completion is unused before going out-of-scope. Fixes: f5d39b020809 ("freezer,sched: Rewrite core freezer logic") Reported-by: syzbot+6cd18e123583550cf469@syzkaller.appspotmail.com Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Debugged-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Link: https://lkml.kernel.org/r/Y90ar35uKQoUrLEK@hirez.programming.kicks-ass.net --- kernel/umh.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/kernel/umh.c b/kernel/umh.c index 8506315..fbf872c 100644 --- a/kernel/umh.c +++ b/kernel/umh.c @@ -438,21 +438,27 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait) if (wait == UMH_NO_WAIT) /* task has freed sub_info */ goto unlock; - if (wait & UMH_KILLABLE) - state |= TASK_KILLABLE; - if (wait & UMH_FREEZABLE) state |= TASK_FREEZABLE; - retval = wait_for_completion_state(&done, state); - if (!retval) - goto wait_done; - if (wait & UMH_KILLABLE) { + retval = wait_for_completion_state(&done, state | TASK_KILLABLE); + if (!retval) + goto wait_done; + /* umh_complete() will see NULL and free sub_info */ if (xchg(&sub_info->complete, NULL)) goto unlock; + + /* + * fallthrough; in case of -ERESTARTSYS now do uninterruptible + * wait_for_completion_state(). Since umh_complete() shall call + * complete() in a moment if xchg() above returned NULL, this + * uninterruptible wait_for_completion_state() will not block + * SIGKILL'ed processes for long. + */ } + wait_for_completion_state(&done, state); wait_done: retval = sub_info->retval; ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [syzbot] WARNING: locking bug in umh_complete 2023-02-03 10:22 ` [syzbot] WARNING: locking bug in umh_complete Tetsuo Handa 2023-02-03 10:48 ` Tetsuo Handa @ 2023-02-03 12:00 ` Peter Zijlstra 1 sibling, 0 replies; 19+ messages in thread From: Peter Zijlstra @ 2023-02-03 12:00 UTC (permalink / raw) To: Tetsuo Handa Cc: Hillf Danton, Ingo Molnar, Rafael J. Wysocki, linux-kernel, mcgrof, Linus Torvalds, syzkaller-bugs, syzbot On Fri, Feb 03, 2023 at 07:22:43PM +0900, Tetsuo Handa wrote: > On 2023/01/27 10:41, Hillf Danton wrote: > >> Call Trace: > >> <TASK> > >> lock_acquire kernel/locking/lockdep.c:5668 [inline] > >> lock_acquire+0x1e3/0x630 kernel/locking/lockdep.c:5633 > >> __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline] > >> _raw_spin_lock_irqsave+0x3d/0x60 kernel/locking/spinlock.c:162 > >> complete+0x1d/0x1f0 kernel/sched/completion.c:32 > >> umh_complete+0x32/0x90 kernel/umh.c:59 > >> call_usermodehelper_exec_sync kernel/umh.c:144 [inline] > >> call_usermodehelper_exec_work+0x115/0x180 kernel/umh.c:167 > >> process_one_work+0x9bf/0x1710 kernel/workqueue.c:2289 > >> worker_thread+0x669/0x1090 kernel/workqueue.c:2436 > >> kthread+0x2e8/0x3a0 kernel/kthread.c:376 > >> ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308 > >> </TASK> > > > > This is an interesting case - given done initialized on stack, no garbage > > should have been detected by lockdep. > > > > One explanation to the report is uaf on the waker side, and it can be > > tested with the diff below when a reproducer is available. > > > > Hillf > > > > --- a/kernel/umh.c > > +++ b/kernel/umh.c > > @@ -452,6 +452,12 @@ int call_usermodehelper_exec(struct subp > > /* umh_complete() will see NULL and free sub_info */ > > if (xchg(&sub_info->complete, NULL)) > > goto unlock; > > + else { > > + /* wait for umh_complete() to finish in a bid to avoid > > + * uaf because done is destructed > > + */ Invalid comment style at the very least. > > + wait_for_completion(&done); > > + } > > } > > > > wait_done: > > -- > > Yes, this bug is caused by commit f5d39b020809 ("freezer,sched: Rewrite core freezer > logic"), for that commit for unknown reason omits wait_for_completion(&done) call > when wait_for_completion_state(&done, state) returned -ERESTARTSYS. > > Peter, is it safe to restore wait_for_completion(&done) call? Urgh, that code is terrible.. the way I read it was that it would wait_for_completion_killable() if KILLABLE and assumed the second wait_for_completion() would NOP out because we'd already completed on the first. I don't see how adding a second wait is correct in the case of -ERESTARTSYS, what's the stop this second wait to also get interrupted like that? Should there be a loop? ^ permalink raw reply [flat|nested] 19+ messages in thread
* [syzbot] WARNING: locking bug in umh_complete @ 2023-01-26 22:27 syzbot 0 siblings, 0 replies; 19+ messages in thread From: syzbot @ 2023-01-26 22:27 UTC (permalink / raw) To: linux-kernel, mcgrof, netdev, syzkaller-bugs Hello, syzbot found the following issue on: HEAD commit: 71ab9c3e2253 net: fix UaF in netns ops registration error .. git tree: net console output: https://syzkaller.appspot.com/x/log.txt?x=10f86a56480000 kernel config: https://syzkaller.appspot.com/x/.config?x=899d86a7610a0ea0 dashboard link: https://syzkaller.appspot.com/bug?extid=6cd18e123583550cf469 compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2 Unfortunately, I don't have any reproducer for this issue yet. Downloadable assets: disk image: https://storage.googleapis.com/syzbot-assets/54c953096fdb/disk-71ab9c3e.raw.xz vmlinux: https://storage.googleapis.com/syzbot-assets/644d72265810/vmlinux-71ab9c3e.xz kernel image: https://storage.googleapis.com/syzbot-assets/16e994579ca5/bzImage-71ab9c3e.xz IMPORTANT: if you fix the issue, please add the following tag to the commit: Reported-by: syzbot+6cd18e123583550cf469@syzkaller.appspotmail.com ------------[ cut here ]------------ DEBUG_LOCKS_WARN_ON(1) WARNING: CPU: 0 PID: 46 at kernel/locking/lockdep.c:231 hlock_class kernel/locking/lockdep.c:231 [inline] WARNING: CPU: 0 PID: 46 at kernel/locking/lockdep.c:231 hlock_class kernel/locking/lockdep.c:220 [inline] WARNING: CPU: 0 PID: 46 at kernel/locking/lockdep.c:231 check_wait_context kernel/locking/lockdep.c:4729 [inline] WARNING: CPU: 0 PID: 46 at kernel/locking/lockdep.c:231 __lock_acquire+0x13ae/0x56d0 kernel/locking/lockdep.c:5005 Modules linked in: CPU: 0 PID: 46 Comm: kworker/u4:3 Not tainted 6.2.0-rc4-syzkaller-00191-g71ab9c3e2253 #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/12/2023 Workqueue: events_unbound call_usermodehelper_exec_work RIP: 0010:hlock_class kernel/locking/lockdep.c:231 [inline] RIP: 0010:hlock_class kernel/locking/lockdep.c:220 [inline] RIP: 0010:check_wait_context kernel/locking/lockdep.c:4729 [inline] RIP: 0010:__lock_acquire+0x13ae/0x56d0 kernel/locking/lockdep.c:5005 Code: 08 84 d2 0f 85 56 41 00 00 8b 15 55 7b 0f 0d 85 d2 0f 85 d5 fd ff ff 48 c7 c6 c0 51 4c 8a 48 c7 c7 20 4b 4c 8a e8 92 e1 5b 08 <0f> 0b 31 ed e9 50 f0 ff ff 48 c7 c0 a8 2b 73 8e 48 ba 00 00 00 00 RSP: 0018:ffffc90000b77a30 EFLAGS: 00010082 RAX: 0000000000000000 RBX: ffff88801272a778 RCX: 0000000000000000 RDX: ffff888012729d40 RSI: ffffffff8166822c RDI: fffff5200016ef38 RBP: 0000000000001b2c R08: 0000000000000005 R09: 0000000000000000 R10: 0000000080000001 R11: 0000000000000001 R12: ffff88801272a7c8 R13: ffff888012729d40 R14: ffffc9000397fb28 R15: 0000000000040000 FS: 0000000000000000(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fa5a95e81d0 CR3: 00000000284d2000 CR4: 00000000003506f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: <TASK> lock_acquire kernel/locking/lockdep.c:5668 [inline] lock_acquire+0x1e3/0x630 kernel/locking/lockdep.c:5633 __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline] _raw_spin_lock_irqsave+0x3d/0x60 kernel/locking/spinlock.c:162 complete+0x1d/0x1f0 kernel/sched/completion.c:32 umh_complete+0x32/0x90 kernel/umh.c:59 call_usermodehelper_exec_sync kernel/umh.c:144 [inline] call_usermodehelper_exec_work+0x115/0x180 kernel/umh.c:167 process_one_work+0x9bf/0x1710 kernel/workqueue.c:2289 worker_thread+0x669/0x1090 kernel/workqueue.c:2436 kthread+0x2e8/0x3a0 kernel/kthread.c:376 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308 </TASK> --- This report is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at syzkaller@googlegroups.com. syzbot will keep track of this issue. See: https://goo.gl/tpsmEJ#status for how to communicate with syzbot. ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2023-02-27 7:59 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20230127014137.4906-1-hdanton@sina.com>
2023-02-03 10:22 ` [syzbot] WARNING: locking bug in umh_complete Tetsuo Handa
2023-02-03 10:48 ` Tetsuo Handa
2023-02-03 12:19 ` Peter Zijlstra
2023-02-03 12:30 ` Peter Zijlstra
2023-02-03 12:59 ` Tetsuo Handa
2023-02-03 14:31 ` Peter Zijlstra
2023-02-04 0:48 ` Tetsuo Handa
2023-02-06 15:51 ` Luis Chamberlain
2023-02-13 15:14 ` Peter Zijlstra
2023-02-13 15:27 ` Peter Zijlstra
2023-02-14 2:31 ` Schspa Shi
2023-02-21 21:54 ` Luis Chamberlain
2023-02-22 9:34 ` Peter Zijlstra
2023-02-27 7:57 ` Schspa Shi
2023-02-13 15:34 ` Peter Zijlstra
2023-02-14 11:16 ` Tetsuo Handa
2023-02-15 10:33 ` [tip: sched/urgent] freezer,umh: Fix call_usermode_helper_exec() vs SIGKILL tip-bot2 for Peter Zijlstra
2023-02-03 12:00 ` [syzbot] WARNING: locking bug in umh_complete Peter Zijlstra
2023-01-26 22:27 syzbot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.