| Message ID | 20250323171955.GA834@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | pidfs: cleanup the usage of do_notify_pidfd() | expand |
On Sun, 23 Mar 2025 18:19:55 +0100, Oleg Nesterov wrote: > If a single-threaded process exits do_notify_pidfd() will be called twice, > from exit_notify() and right after that from do_notify_parent(). > > 1. Change exit_notify() to call do_notify_pidfd() if the exiting task is > not ptraced and it is not a group leader. > > 2. Change do_notify_parent() to call do_notify_pidfd() unconditionally. > > [...] Applied to the vfs.fixes branch of the vfs/vfs.git tree. Patches in the vfs.fixes branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.fixes [1/1] pidfs: cleanup the usage of do_notify_pidfd() https://git.kernel.org/vfs/vfs/c/98ce463bc6f4 [1/1] selftests/pidfd: (Was: [PATCH] pidfs: cleanup the usage of do_notify_pidfd()) https://git.kernel.org/vfs/vfs/c/cc8c2e338a25
On Sun, Mar 23, 2025 at 06:19:55PM +0100, Oleg Nesterov wrote: > If a single-threaded process exits do_notify_pidfd() will be called twice, > from exit_notify() and right after that from do_notify_parent(). > > 1. Change exit_notify() to call do_notify_pidfd() if the exiting task is > not ptraced and it is not a group leader. > > 2. Change do_notify_parent() to call do_notify_pidfd() unconditionally. > > If tsk is not ptraced, do_notify_parent() will only be called when it > is a group-leader and thread_group_empty() is true. > > This means that if tsk is ptraced, do_notify_pidfd() will be called from > do_notify_parent() even if tsk is a delay_group_leader(). But this case is > less common, and apart from the unnecessary __wake_up() is harmless. > > Granted, this unnecessary __wake_up() can be avoided, but I don't want to > do it in this patch because it's just a consequence of another historical > oddity: we notify the tracer even if !thread_group_empty(), but do_wait() > from debugger can't work until all other threads exit. With or without this > patch we should either eliminate do_notify_parent() in this case, or change > do_wait(WEXITED) to untrace the ptraced delay_group_leader() at least when > ptrace_reparented(). > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> > --- Thanks for doing this! I'll send this together with the first set of fixes after the merge window closes. > kernel/exit.c | 8 ++------ > kernel/signal.c | 8 +++----- > 2 files changed, 5 insertions(+), 11 deletions(-) > > diff --git a/kernel/exit.c b/kernel/exit.c > index 683766316a3d..d0ebccb9dec0 100644 > --- a/kernel/exit.c > +++ b/kernel/exit.c > @@ -742,12 +742,6 @@ static void exit_notify(struct task_struct *tsk, int group_dead) > kill_orphaned_pgrp(tsk->group_leader, NULL); > > tsk->exit_state = EXIT_ZOMBIE; > - /* > - * Ignore thread-group leaders that exited before all > - * subthreads did. > - */ > - if (!delay_group_leader(tsk)) > - do_notify_pidfd(tsk); > > if (unlikely(tsk->ptrace)) { > int sig = thread_group_leader(tsk) && > @@ -760,6 +754,8 @@ static void exit_notify(struct task_struct *tsk, int group_dead) > do_notify_parent(tsk, tsk->exit_signal); > } else { > autoreap = true; > + /* untraced sub-thread */ > + do_notify_pidfd(tsk); > } > > if (autoreap) { > diff --git a/kernel/signal.c b/kernel/signal.c > index 027ad9e97417..1d8db0dabb71 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -2179,11 +2179,9 @@ bool do_notify_parent(struct task_struct *tsk, int sig) > > WARN_ON_ONCE(!tsk->ptrace && > (tsk->group_leader != tsk || !thread_group_empty(tsk))); > - /* > - * Notify for thread-group leaders without subthreads. > - */ > - if (thread_group_empty(tsk)) > - do_notify_pidfd(tsk); > + > + /* ptraced, or group-leader without sub-threads */ > + do_notify_pidfd(tsk); > > if (sig != SIGCHLD) { > /* > -- > 2.25.1.362.g51ebf55 > >
diff --git a/kernel/exit.c b/kernel/exit.c index 683766316a3d..d0ebccb9dec0 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -742,12 +742,6 @@ static void exit_notify(struct task_struct *tsk, int group_dead) kill_orphaned_pgrp(tsk->group_leader, NULL); tsk->exit_state = EXIT_ZOMBIE; - /* - * Ignore thread-group leaders that exited before all - * subthreads did. - */ - if (!delay_group_leader(tsk)) - do_notify_pidfd(tsk); if (unlikely(tsk->ptrace)) { int sig = thread_group_leader(tsk) && @@ -760,6 +754,8 @@ static void exit_notify(struct task_struct *tsk, int group_dead) do_notify_parent(tsk, tsk->exit_signal); } else { autoreap = true; + /* untraced sub-thread */ + do_notify_pidfd(tsk); } if (autoreap) { diff --git a/kernel/signal.c b/kernel/signal.c index 027ad9e97417..1d8db0dabb71 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -2179,11 +2179,9 @@ bool do_notify_parent(struct task_struct *tsk, int sig) WARN_ON_ONCE(!tsk->ptrace && (tsk->group_leader != tsk || !thread_group_empty(tsk))); - /* - * Notify for thread-group leaders without subthreads. - */ - if (thread_group_empty(tsk)) - do_notify_pidfd(tsk); + + /* ptraced, or group-leader without sub-threads */ + do_notify_pidfd(tsk); if (sig != SIGCHLD) { /*
If a single-threaded process exits do_notify_pidfd() will be called twice, from exit_notify() and right after that from do_notify_parent(). 1. Change exit_notify() to call do_notify_pidfd() if the exiting task is not ptraced and it is not a group leader. 2. Change do_notify_parent() to call do_notify_pidfd() unconditionally. If tsk is not ptraced, do_notify_parent() will only be called when it is a group-leader and thread_group_empty() is true. This means that if tsk is ptraced, do_notify_pidfd() will be called from do_notify_parent() even if tsk is a delay_group_leader(). But this case is less common, and apart from the unnecessary __wake_up() is harmless. Granted, this unnecessary __wake_up() can be avoided, but I don't want to do it in this patch because it's just a consequence of another historical oddity: we notify the tracer even if !thread_group_empty(), but do_wait() from debugger can't work until all other threads exit. With or without this patch we should either eliminate do_notify_parent() in this case, or change do_wait(WEXITED) to untrace the ptraced delay_group_leader() at least when ptrace_reparented(). Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- kernel/exit.c | 8 ++------ kernel/signal.c | 8 +++----- 2 files changed, 5 insertions(+), 11 deletions(-)