| Message ID | 20220518225355.784371-5-ebiederm@xmission.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | ptrace: cleanups and calling do_cldstop with only siglock | expand |
Sorry for delay. On 05/18, Eric W. Biederman wrote: > > Ever since commit 28d838cc4dfe ("Fix ptrace self-attach rule") it has > been impossible to attach another thread in the same thread group. > > Remove the code from __ptrace_detach that was trying to support > detaching from a thread in the same thread group. may be I am totally confused, but I think you misunderstood this code and thus this patch is very wrong. The same_thread_group() check does NOT try to check if debugger and tracee is in the same thread group, this is indeed impossible. We need this check to know if the tracee was ptrace_reparented() before __ptrace_unlink() or not. > -static int ignoring_children(struct sighand_struct *sigh) > -{ > - int ret; > - spin_lock(&sigh->siglock); > - ret = (sigh->action[SIGCHLD-1].sa.sa_handler == SIG_IGN) || > - (sigh->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDWAIT); > - spin_unlock(&sigh->siglock); > - return ret; > -} ... > @@ -565,14 +552,9 @@ static bool __ptrace_detach(struct task_struct *tracer, struct task_struct *p) > > dead = !thread_group_leader(p); > > - if (!dead && thread_group_empty(p)) { > - if (!same_thread_group(p->real_parent, tracer)) > - dead = do_notify_parent(p, p->exit_signal); > - else if (ignoring_children(tracer->sighand)) { > - __wake_up_parent(p, tracer); > - dead = true; > - } > - } So the code above does: - if !same_thread_group(p->real_parent, tracer), then the tracee was ptrace_reparented(), and now we need to notify its natural parent to let it know it has a zombie child. - otherwise, the tracee is our natural child, and it is actually dead. however, since we are going to reap this task, we need to wake up our sub-threads possibly sleeping on ->wait_chldexit wait_queue_head_t. See? > + if (!dead && thread_group_empty(p)) > + dead = do_notify_parent(p, p->exit_signal); No, this looks wrong. Or I missed something? Oleg.
On 05/24, Oleg Nesterov wrote: > > Sorry for delay. > > On 05/18, Eric W. Biederman wrote: > > > > Ever since commit 28d838cc4dfe ("Fix ptrace self-attach rule") it has > > been impossible to attach another thread in the same thread group. > > > > Remove the code from __ptrace_detach that was trying to support > > detaching from a thread in the same thread group. > > may be I am totally confused, but I think you misunderstood this code > and thus this patch is very wrong. > > The same_thread_group() check does NOT try to check if debugger and > tracee is in the same thread group, this is indeed impossible. > > We need this check to know if the tracee was ptrace_reparented() before > __ptrace_unlink() or not. > > > > -static int ignoring_children(struct sighand_struct *sigh) > > -{ > > - int ret; > > - spin_lock(&sigh->siglock); > > - ret = (sigh->action[SIGCHLD-1].sa.sa_handler == SIG_IGN) || > > - (sigh->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDWAIT); > > - spin_unlock(&sigh->siglock); > > - return ret; > > -} > > ... > > > @@ -565,14 +552,9 @@ static bool __ptrace_detach(struct task_struct *tracer, struct task_struct *p) > > > > dead = !thread_group_leader(p); > > > > - if (!dead && thread_group_empty(p)) { > > - if (!same_thread_group(p->real_parent, tracer)) > > - dead = do_notify_parent(p, p->exit_signal); > > - else if (ignoring_children(tracer->sighand)) { > > - __wake_up_parent(p, tracer); > > - dead = true; > > - } > > - } > > So the code above does: > > - if !same_thread_group(p->real_parent, tracer), then the tracee was > ptrace_reparented(), and now we need to notify its natural parent > to let it know it has a zombie child. > > - otherwise, the tracee is our natural child, and it is actually dead. > however, since we are going to reap this task, we need to wake up our > sub-threads possibly sleeping on ->wait_chldexit wait_queue_head_t. > > See? > > > + if (!dead && thread_group_empty(p)) > > + dead = do_notify_parent(p, p->exit_signal); > > No, this looks wrong. Or I missed something? Yes, but... That said, it seems that we do not need __wake_up_parent() if it was our natural child? I'll recheck. Eric, I'll continue to read this series tomorrow, can't concentrate on ptrace today. Oleg.
Oleg Nesterov <oleg@redhat.com> writes: > On 05/24, Oleg Nesterov wrote: >> >> Sorry for delay. >> >> On 05/18, Eric W. Biederman wrote: >> > >> > Ever since commit 28d838cc4dfe ("Fix ptrace self-attach rule") it has >> > been impossible to attach another thread in the same thread group. >> > >> > Remove the code from __ptrace_detach that was trying to support >> > detaching from a thread in the same thread group. >> >> may be I am totally confused, but I think you misunderstood this code >> and thus this patch is very wrong. >> >> The same_thread_group() check does NOT try to check if debugger and >> tracee is in the same thread group, this is indeed impossible. >> >> We need this check to know if the tracee was ptrace_reparented() before >> __ptrace_unlink() or not. >> >> >> > -static int ignoring_children(struct sighand_struct *sigh) >> > -{ >> > - int ret; >> > - spin_lock(&sigh->siglock); >> > - ret = (sigh->action[SIGCHLD-1].sa.sa_handler == SIG_IGN) || >> > - (sigh->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDWAIT); >> > - spin_unlock(&sigh->siglock); >> > - return ret; >> > -} >> >> ... >> >> > @@ -565,14 +552,9 @@ static bool __ptrace_detach(struct task_struct *tracer, struct task_struct *p) >> > >> > dead = !thread_group_leader(p); >> > >> > - if (!dead && thread_group_empty(p)) { >> > - if (!same_thread_group(p->real_parent, tracer)) >> > - dead = do_notify_parent(p, p->exit_signal); >> > - else if (ignoring_children(tracer->sighand)) { >> > - __wake_up_parent(p, tracer); >> > - dead = true; >> > - } >> > - } >> >> So the code above does: >> >> - if !same_thread_group(p->real_parent, tracer), then the tracee was >> ptrace_reparented(), and now we need to notify its natural parent >> to let it know it has a zombie child. >> >> - otherwise, the tracee is our natural child, and it is actually dead. >> however, since we are going to reap this task, we need to wake up our >> sub-threads possibly sleeping on ->wait_chldexit wait_queue_head_t. >> >> See? >> >> > + if (!dead && thread_group_empty(p)) >> > + dead = do_notify_parent(p, p->exit_signal); >> >> No, this looks wrong. Or I missed something? > > Yes, but... > > That said, it seems that we do not need __wake_up_parent() if it was our > natural child? Agreed on both counts. Hmm. I see where the logic comes from. The ignoring_children test and the __wake_up_parent are what do_notify_parent does when the parent ignores children. Hmm. I even see all of this document in the comment above __ptrace_detach. So I am just going to drop this change. > I'll recheck. Eric, I'll continue to read this series tomorrow, can't > concentrate on ptrace today. No worries. This was entirely too close to the merge window so I dropped it all until today. Eric
diff --git a/kernel/ptrace.c b/kernel/ptrace.c index 328a34a99124..ca0e47691229 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -526,19 +526,6 @@ static int ptrace_traceme(void) return ret; } -/* - * Called with irqs disabled, returns true if childs should reap themselves. - */ -static int ignoring_children(struct sighand_struct *sigh) -{ - int ret; - spin_lock(&sigh->siglock); - ret = (sigh->action[SIGCHLD-1].sa.sa_handler == SIG_IGN) || - (sigh->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDWAIT); - spin_unlock(&sigh->siglock); - return ret; -} - /* * Called with tasklist_lock held for writing. * Unlink a traced task, and clean it up if it was a traced zombie. @@ -565,14 +552,9 @@ static bool __ptrace_detach(struct task_struct *tracer, struct task_struct *p) dead = !thread_group_leader(p); - if (!dead && thread_group_empty(p)) { - if (!same_thread_group(p->real_parent, tracer)) - dead = do_notify_parent(p, p->exit_signal); - else if (ignoring_children(tracer->sighand)) { - __wake_up_parent(p, tracer); - dead = true; - } - } + if (!dead && thread_group_empty(p)) + dead = do_notify_parent(p, p->exit_signal); + /* Mark it as in the process of being reaped. */ if (dead) p->exit_state = EXIT_DEAD;
Ever since commit 28d838cc4dfe ("Fix ptrace self-attach rule") it has been impossible to attach another thread in the same thread group. Remove the code from __ptrace_detach that was trying to support detaching from a thread in the same thread group. The code is dead and I can not make sense of what it is trying to do. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- kernel/ptrace.c | 24 +++--------------------- 1 file changed, 3 insertions(+), 21 deletions(-)