Re: [PATCH ptrace] ptrace: fix signal->wait_chldexit usage in task_clear_group_stop_trapping()

[Tejun Heo <tj@kernel.org>]

Hello, Oleg.

On Sun, May 08, 2011 at 03:35:43PM +0200, Oleg Nesterov wrote:
> > Given the relative low frequency of ptrace use, we would be much
> > better off leaving already complex wait_chldexit alone and using bit
> > waitqueue.
> 
> Well, I don't think so. wait_on_bit() looks as unnecessary complication
> to me. See below.

Why is wait_on_bit() a complication?  It's a well defined event
construct.

> But. Why do we need signal->wait_chldexit or bit waitqueue at all?
> Previously this was needed because wait_event(!GROUP_STOP_TRAPPING)
> was called from ptrace_check_attach(), and the tracer can do anything
> after ptrace_attach() which sets GROUP_STOP_TRAPPING.
>
> With the current code we know that GROUP_STOP_TRAPPING means: the
> tracer can't go away from ptrace_attach() until we clear this bit.

Several reasons.

* Because I'm gonna use TRAPPING for end of group stop notification
  too and move TRAPPING waiting to ptrace_check_attach() and
  wait_task_stopped().

* I dislike adding unqualified wake_up_process() unless the
  interlocked behavior with the waiter is very obvious.  The waiter is
  waiting for an event to happen and the waker is generating the
  event.  It's much more clear and robust to describe that
  relationship explicitly rather than depending on "on yeah, if the
  waiter wasn't expecting this event yet, other sleeping places are
  supposed to be safe against spurious wakeups so we're all good".

* I think it's a bad idea to design such tightly coupled waiter/waker
  relationship as in "because the waiter is not gonna do anything
  else, just waking up is safe".  It's way too fragile and prone to
  error when code gets updated later and it's not like we gain
  anything from it.  Let's make waiter wait for a well defined event
  and waker generate that event.  We have pretty good facilities for
  that kind of things.

> Hmm. This is minor and off-topic, but perhaps it makes sense to move
> the code which sets/waits GROUP_STOP_TRAPPING from ptrace_attach() to
> the separate function, it can be called outside of tasklist_lock and
> cred_guard_mutex.

I'm confused.  It should be set while siglock is held.  Which place
are you suggesting?

> And. Could you remind why ptrace_attach() does signal_wake_up() instead
> of wake_up_state(TASK_STOPPED) ? OK, in general we shouldn't set
> GROUP_STOP_PENDING without TIF_SIGPENDING, but in this case?

I don't know.  I can't find any good reason there.  Feel free to
change it to wake_up_state(TASK_STOPPED) but leaving it as
signal_wake_up() would look prettier after SEIZE addition, and I think
using the same @resume abstraction for all ptrace wakeups make things
easier to understand too.  I'll soon post the patchset.

Thanks.

-- 
tejun