| Message ID | 20240608120616.GB7947@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7fea700e04bd3f424c2d836e98425782f97b494e |
| Headers | show |
| Series | zap_pid_ns_processes: clear TIF_NOTIFY_SIGNAL along with TIF_SIGPENDING | expand |
On Sat, Jun 08, 2024 at 02:06:16PM +0200, Oleg Nesterov wrote: > kernel_wait4() doesn't sleep and returns -EINTR if there is no > eligible child and signal_pending() is true. > > That is why zap_pid_ns_processes() clears TIF_SIGPENDING but this is not > enough, it should also clear TIF_NOTIFY_SIGNAL to make signal_pending() > return false and avoid a busy-wait loop. > > Fixes: 12db8b690010 ("entry: Add support for TIF_NOTIFY_SIGNAL") > Reported-by: Rachel Menge <rachelmenge@linux.microsoft.com> > Closes: https://lore.kernel.org/all/1386cd49-36d0-4a5c-85e9-bc42056a5a38@linux.microsoft.com/ > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Reviewed-by: Boqun Feng <boqun.feng@gmail.com> Wei, appreciate it if you could share some test result and provide a Tested-by tag. Thanks! Regards, Boqun > --- > kernel/pid_namespace.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c > index dc48fecfa1dc..25f3cf679b35 100644 > --- a/kernel/pid_namespace.c > +++ b/kernel/pid_namespace.c > @@ -218,6 +218,7 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns) > */ > do { > clear_thread_flag(TIF_SIGPENDING); > + clear_thread_flag(TIF_NOTIFY_SIGNAL); > rc = kernel_wait4(-1, NULL, __WALL, NULL); > } while (rc != -ECHILD); > > -- > 2.25.1.362.g51ebf55 > >
> kernel_wait4() doesn't sleep and returns -EINTR if there is no > eligible child and signal_pending() is true. > > That is why zap_pid_ns_processes() clears TIF_SIGPENDING but this is not > enough, it should also clear TIF_NOTIFY_SIGNAL to make signal_pending() > return false and avoid a busy-wait loop. > > Fixes: 12db8b690010 ("entry: Add support for TIF_NOTIFY_SIGNAL") > Reported-by: Rachel Menge <rachelmenge@linux.microsoft.com> > Closes: https://lore.kernel.org/all/1386cd49-36d0-4a5c-85e9-bc42056a5a38@linux.microsoft.com/ > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Tested-By: Wei Fu <fuweid89@gmail.com> This change looks good to me! I used [rcudeadlock-v1][1] to verify this patch on v5.15.160 for more than 30 hours. The soft lockup didn't show up. If there is no such patch, that test will trigger soft-lockup in 10 minutes. ``` root@(none):/# uname -a Linux (none) 5.15.160-dirty #7 SMP Fri Jun 7 15:25:30 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux root@(none):/# ps -ef | grep rcu root 3 2 0 Jun07 ? 00:00:00 [rcu_gp] root 4 2 0 Jun07 ? 00:00:00 [rcu_par_gp] root 11 2 0 Jun07 ? 00:00:00 [rcu_tasks_rude_] root 12 2 0 Jun07 ? 00:00:00 [rcu_tasks_trace] root 15 2 0 Jun07 ? 00:03:31 [rcu_sched] root 145 141 0 Jun07 ? 00:15:29 ./rcudeadlock root 5372 141 0 13:37 ? 00:00:00 grep rcu root@(none):/# date Sun Jun 9 13:37:38 UTC 2024 ``` I used [rcudeadlock-v2][2] to verify this patch on v6.10-rc2 for more than 2 hours. The soft lockup didn't show up. If there is no such patch, that test will trigger soft-lockup in 1 minute. ``` root@(none):/# uname -a Linux (none) 6.10.0-rc2-dirty #4 SMP Sun Jun 9 11:19:40 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux root@(none):/# ps -ef | grep rcu root 4 2 0 11:20 ? 00:00:00 [kworker/R-rcu_g] root 13 2 0 11:20 ? 00:00:00 [rcu_tasks_rude_kthread] root 14 2 0 11:20 ? 00:00:00 [rcu_tasks_trace_kthread] root 16 2 0 11:20 ? 00:00:03 [rcu_sched] root 17 2 0 11:20 ? 00:00:00 [rcu_exp_par_gp_kthread_worker/0] root 18 2 0 11:20 ? 00:00:12 [rcu_exp_gp_kthread_worker] root 117 108 0 11:21 ? 00:01:06 ./rcudeadlock root 14451 108 0 13:37 ? 00:00:00 grep rcu root@(none):/# date Sun Jun 9 13:37:15 UTC 2024 ``` It's about data-race during cleanup active iou-wrk-thread. I shares that idea about how to verify this patch. > --- > kernel/pid_namespace.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c > index dc48fecfa1dc..25f3cf679b35 100644 > --- a/kernel/pid_namespace.c > +++ b/kernel/pid_namespace.c > @@ -218,6 +218,7 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns) > */ > do { > clear_thread_flag(TIF_SIGPENDING); > + clear_thread_flag(TIF_NOTIFY_SIGNAL); > rc = kernel_wait4(-1, NULL, __WALL, NULL); > } while (rc != -ECHILD); > > -- > 2.25.1.362.g51ebf55 > > > Let's assume that there is new pid namespace unshared by host pid namespace, named by `PA`. There are two processes in `PA`. The init process is named by `X` and its child is named by `Y`. ``` unshare(CLONE_NEWPID|CLONE_NEWNS) X |__ Y ``` The main-thread of process X creates one active iouring worker thread `iou-wrk-X`. When process X exits, that main-thread of process X wakes up and set `TIF_NOTIFY_SIGNAL` flag on `iou-wrk-X` thread. However, if `iou-wrk-X` thread receives signal from main-thread and wakes up, that thread isn't able to clear `TIF_NOTIFY_SIGNAL` flag. And that `iou-wrk-X` thread is last thread in process-X and it will carry `TIF_NOTIFY_SIGNAL` flag to enter `zap_pid_ns_processes`. It can be described by the following comment. ``` == X main-thread == == X iou-wrk-X == == Y main-thread == do_exit kill iou-wrk-X thread io_uring_files_cancel io_wq_worker set TIF_NOTIFY_SIGNAL on iou-wrk-X thread do_exit(0) exit_task_namespace exit_task_namespace do_task_dead exit_notify forget_original_parent find_child_reaper zap_pid_ns_processes do_exit exit_task_namespace ... namespace_unlock synchronize_rcu_expedited ``` The `iou-wrk-X` thread kills process-Y which is only one holding the mount namespace reference. The process-Y will get into `synchronize_rcu_expedited`. Since kernel doesn't enable preempt and `iou-wrk-X` thread has `TIF_NOTIFY_SIGNAL` flag, the `iou-wrk-X` thread will get into infinity loop, which cause soft lockup. So, in [rcudeadlock-v2][2] test, I create more active iou-wrk- threads in init process so that there is high chance to have iou-wrk- thread in `zap_pid_ns_processes` function. Hope it can help. Thanks, Wei [1]: https://github.com/rlmenge/rcu-soft-lock-issue-repro/blob/662b8e414ff15d75419e2286b8121b7c2049a37c/rcudeadlock.go#L1 [2]: https://github.com/rlmenge/rcu-soft-lock-issue-repro/pull/1
On 6/8/24 6:06 AM, Oleg Nesterov wrote: > kernel_wait4() doesn't sleep and returns -EINTR if there is no > eligible child and signal_pending() is true. > > That is why zap_pid_ns_processes() clears TIF_SIGPENDING but this is not > enough, it should also clear TIF_NOTIFY_SIGNAL to make signal_pending() > return false and avoid a busy-wait loop. Reviewed-by: Jens Axboe <axboe@kernel.dk>
Oleg Nesterov <oleg@redhat.com> writes: > kernel_wait4() doesn't sleep and returns -EINTR if there is no > eligible child and signal_pending() is true. > > That is why zap_pid_ns_processes() clears TIF_SIGPENDING but this is not > enough, it should also clear TIF_NOTIFY_SIGNAL to make signal_pending() > return false and avoid a busy-wait loop. I took a look through the code. It used to be that TIF_NOTIFY_SIGNAL was all about waking up a task so that task_work_run can be used. io_uring still mostly uses it that way. There is also a use in kthread_stop that just uses it as a TIF_SIGPENDING without having a pending signal. At the point in do_exit where exit_notify and thus zap_pid_ns_processes is called I can't possibly see a use for TIF_NOTIFY_SIGNAL. exit_task_work, exit_signals, and io_uring_cancel have all been called. So TIF_NOTIFY_SIGNAL should be spurious at this point and safe to clear. Why it remains set is a mystery to me. If I had infinite time and energy the ideal is to rework the pid namespace exit logic so that waiting for everything to exit works like delay_group_leader in wait_task_consider. Simply blocking reaping of the pid namespace leader until everything in the pid namespace have been reaped. I think acct_exit_ns is the only piece of code that needs to be moved to allow that, and acct_exit_ns is purely bookkeeping so does not affect userspace visible semantics. This active waiting is weird and non-standard in the kernel and winds up causeing a problem every couple of years because of that. > > Fixes: 12db8b690010 ("entry: Add support for TIF_NOTIFY_SIGNAL") > Reported-by: Rachel Menge <rachelmenge@linux.microsoft.com> > Closes: https://lore.kernel.org/all/1386cd49-36d0-4a5c-85e9-bc42056a5a38@linux.microsoft.com/ > Signed-off-by: Oleg Nesterov <oleg@redhat.com> > --- > kernel/pid_namespace.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c > index dc48fecfa1dc..25f3cf679b35 100644 > --- a/kernel/pid_namespace.c > +++ b/kernel/pid_namespace.c > @@ -218,6 +218,7 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns) > */ > do { > clear_thread_flag(TIF_SIGPENDING); > + clear_thread_flag(TIF_NOTIFY_SIGNAL); > rc = kernel_wait4(-1, NULL, __WALL, NULL); > } while (rc != -ECHILD);
> > Oleg Nesterov <oleg@redhat.com> writes: > > > kernel_wait4() doesn't sleep and returns -EINTR if there is no > > eligible child and signal_pending() is true. > > > > That is why zap_pid_ns_processes() clears TIF_SIGPENDING but this is not > > enough, it should also clear TIF_NOTIFY_SIGNAL to make signal_pending() > > return false and avoid a busy-wait loop. > > I took a look through the code. It used to be that TIF_NOTIFY_SIGNAL > was all about waking up a task so that task_work_run can be used. > io_uring still mostly uses it that way. There is also a use in > kthread_stop that just uses it as a TIF_SIGPENDING without having a > pending signal. > > At the point in do_exit where exit_notify and thus zap_pid_ns_processes > is called I can't possibly see a use for TIF_NOTIFY_SIGNAL. > exit_task_work, exit_signals, and io_uring_cancel have all been called. > > So TIF_NOTIFY_SIGNAL should be spurious at this point and safe to clear. > Why it remains set is a mystery to me. I think there is a case that TIF_NOTIFY_SIGNAL remains set. Init process has main-thread, sub-thread-X and iou-wrk-thread-X (created by sub-thread-X). When main-thread enters exit_group, both sub-thread-X and iou-wrk-thread-X are set by TIF_SIGPENDING and wake up. The sub-thread-X could call io_uring_cancel to set TIF_NOTIFY_SIGNAL for iou-wrk-thread-X which doesn't have chance to clear it. And then iou-wrk-thread-X gets into zap_pid_ns_processes function with TIF_NOTIFY_SIGNAL flag. If there are active processes in that pid namespace, it will run into this issue. Wei
On 06/13, Wei Fu wrote: > > I think there is a case that TIF_NOTIFY_SIGNAL remains set. [...snip...] Of course! but please forget about io_uring even if currently io_uring/ is the only user of TWA_SIGNAL. Just suppose that the exiting task/thread races with task_workd_add(TWA_SIGNAL), TIF_NOTIFY_SIGNAL won't be cleared. This is fine in that the exiting task T will do exit_task_work() and after that task_work_add(T) can't succeed with or without TWA_SIGNAL. So it can't miss the pending work. But I think we can forget about TIF_NOTIFY_SIGNAL. To me, the problem is that the state of signal_pending() of the exiting task was never clearly defined, and I can't even recall how many times I mentioned this in the previous discussions. TIF_NOTIFY_SIGNAL doesn't add more confusion, imo. Oleg.
On 06/13, Eric W. Biederman wrote: > > Oleg Nesterov <oleg@redhat.com> writes: > > > kernel_wait4() doesn't sleep and returns -EINTR if there is no > > eligible child and signal_pending() is true. > > > > That is why zap_pid_ns_processes() clears TIF_SIGPENDING but this is not > > enough, it should also clear TIF_NOTIFY_SIGNAL to make signal_pending() > > return false and avoid a busy-wait loop. > > I took a look through the code. It used to be that TIF_NOTIFY_SIGNAL > was all about waking up a task so that task_work_run can be used. > io_uring still mostly uses it that way. There is also a use in > kthread_stop that just uses it as a TIF_SIGPENDING without having a > pending signal. > > At the point in do_exit where exit_notify and thus zap_pid_ns_processes > is called I can't possibly see a use for TIF_NOTIFY_SIGNAL. > exit_task_work, exit_signals, and io_uring_cancel have all been called. > > So TIF_NOTIFY_SIGNAL should be spurious at this point and safe to clear. > Why it remains set is a mystery to me. because exit_task_work() -> task_work_run() doesn't clear TIF_NOTIFY_SIGNAL. So yes, it is spurious, but to me a possible TIF_SIGPENDING is even more "spurious". See my reply to Wei. We don't need to clear TIF_NOTIFY_SIGNAL inside the loop, task_work_addd() can't succeed after exit_task_work() sets ->task_works =&work_exited, but this is another story and this doesn't (well, shouldn't) differ from TIF_SIGPENDING. > If I had infinite time and energy the ideal is to rework the pid > namespace exit logic Perhaps in this case you could take a look at the next loop waiting for pid_ns->pid_allocated == init_pids ;) I always hated the fact that the the exiting sub-namespace init can "hang forever" if this namespace has the tasks injected from the parent namespace. And I had enough hard-to-debug internal bug reports which blamed the kernel. > This active waiting is weird and non-standard in the kernel and winds up > causeing a problem every couple of years because of that. Agreed. Oleg.
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c index dc48fecfa1dc..25f3cf679b35 100644 --- a/kernel/pid_namespace.c +++ b/kernel/pid_namespace.c @@ -218,6 +218,7 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns) */ do { clear_thread_flag(TIF_SIGPENDING); + clear_thread_flag(TIF_NOTIFY_SIGNAL); rc = kernel_wait4(-1, NULL, __WALL, NULL); } while (rc != -ECHILD);
kernel_wait4() doesn't sleep and returns -EINTR if there is no eligible child and signal_pending() is true. That is why zap_pid_ns_processes() clears TIF_SIGPENDING but this is not enough, it should also clear TIF_NOTIFY_SIGNAL to make signal_pending() return false and avoid a busy-wait loop. Fixes: 12db8b690010 ("entry: Add support for TIF_NOTIFY_SIGNAL") Reported-by: Rachel Menge <rachelmenge@linux.microsoft.com> Closes: https://lore.kernel.org/all/1386cd49-36d0-4a5c-85e9-bc42056a5a38@linux.microsoft.com/ Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- kernel/pid_namespace.c | 1 + 1 file changed, 1 insertion(+)