Messages in this thread | | | Date | Tue, 21 Dec 2010 17:31:07 +0100 | From | Tejun Heo <> | Subject | Re: [PATCH 02/16] signal: fix CLD_CONTINUED notification target |
| |
Hello, Oleg.
On Mon, Dec 20, 2010 at 03:58:15PM +0100, Oleg Nesterov wrote: > On 12/06, Tejun Heo wrote: > > > > CLD_CONTINUED notification code calls do_notify_parent_cldstop() with > > its group_leader; however, do_notify_parent_cldstop() already uses the > > group_leader for non-ptraced notifications. > > Yes, > > > The duplicate > > ->group_leader dereferencing is unnecessary and leads to incorrectly > > notifying the group_leader's ptracer of CLD_CONTINUED from a different > > task in the group. Fix it. > > I do not really agree this is wrong, group_leader was used intentionally > for ptrace case. > > There is no "correct" thread who should report CLD_CONTINUED, a random > thread wins and notifies its ->real_parent or debugger. If we always > choose ->group_leader, we always knows what happens. With this patch > we can't predict where does this notification go. > > > --- a/kernel/signal.c > > +++ b/kernel/signal.c > > @@ -1867,7 +1867,7 @@ relock: > > > > if (why) { > > read_lock(&tasklist_lock); > > - do_notify_parent_cldstop(current->group_leader, why); > > + do_notify_parent_cldstop(current, why); > > OTOH, I see nothing really wrong with this change, and this all will > be reworked by the next patches anyway.
Ah, okay, I thought it was an unintentional bug. The reason why the change is necessary is because of later changes which re-instates the notification on ptrace detach. The code is modified such that notification pending is cleared iff the notification is delivered to an actual parent. The double dereference makes it difficult to tell whether the notification is beling delivered to an actual parent or not.
If this wasn't a bug, I think the proper thing to do is to move this later where the change is necessary, after the code learns how to buffer the notification until ptrace detach and needs to determine whether it's being eaten by the debugger or not.
Thanks.
-- tejun
| |