Messages in this thread | | | Date | Wed, 22 Dec 2010 12:54:09 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH 12/16] ptrace: make group stop notification reliable against ptrace |
| |
On 12/21, Tejun Heo wrote: > > On Mon, Dec 20, 2010 at 06:34:25PM +0100, Oleg Nesterov wrote: > > On 12/06, Tejun Heo wrote: > > > > > > This patch adds a new signal flag SIGNAL_NOTIFY_STOP which is set on > > > group stop completion and cleared on notification to the real parent > > > or together with other stopped flags on SIGCONT/KILL. This guarantees > > > that the real parent is notified correctly regardless of ptrace. > > > > OK, I am a bit confused. I do not understand exactly what this > > "correctly" actually means. > > It means that the ptracer wouldn't eat the notification. The > notification is buffered and delivered when ptrace detaches.
Yes, I understand. I am a bit worried it is not easy to describe the new behaviour exactly.
> I see. My focus was to make ptrace attach/detach transparent. IOW, > minimizing the effect of a debugger (or gcore or whatever) attaching > and then leaving. So, this patch just makes sure that the > notification isn't absorbed by a ptracer.
Agreed. And the code itself certainly becomes correct/consistent, contrary to "everything is broken" we currently have.
Tejun, I'll try to summarize my (very foggy) concerns in a separate email. Don't get me wrong, I think this series rightly addresses the numerous problems we have. My only question is, can't we go a bit further and create the new (and simple) rules. Probably not.
> > > @@ -1901,21 +1925,12 @@ retry: > > > __set_current_state(TASK_STOPPED); > > > > > > if (likely(!task_ptrace(current))) { > > > - int notify = 0; > > > - > > > - /* > > > - * If there are no other threads in the group, or if there > > > - * is a group stop in progress and we are the last to stop, > > > - * report to the parent. > > > - */ > > > - if (task_participate_group_stop(current)) > > > - notify = CLD_STOPPED; > > > - > > > + task_participate_group_stop(current); > > > spin_unlock_irq(¤t->sighand->siglock); > > > > > > - if (notify) { > > > + if (sig->flags & SIGNAL_NOTIFY_STOP) { > > > read_lock(&tasklist_lock); > > > - do_notify_parent_cldstop(current, notify); > > > + do_notify_parent_cldstop(current, CLD_STOPPED); > > > > Suppose that debugger attaches right after spin_unlock(->siglock). > > > > Nothing really bad can happen afaics, but in this case the debugger > > will be notified twice. Hmm. If the debugger does do_wait() immediately > > after the first notification, it has all rights to see the stopped > > tracee but wait_task_stopped() fails, not good. > > Hmmm? ptrace_attach() can't happen while tasklist_lock is held.
Sure, but is is not held after we drop ->siglock. And ptrace_attach() can happen in the window before we take it for do_notify_parent_cldstop().
Oleg.
| |