Re: ptrace fixes for 3.2

[Tejun Heo <tj@kernel.org>]

Hello,

On Wed, Jan 04, 2012 at 12:35:34PM +0100, Oleg Nesterov wrote:
> Can't understand how did we miss this, but WARN_ON_ONCE(!ptrace)
> in do_signal_stop() is not right. Debugger can resume the stopped
> task, and it can clone the _untraced_ thread running in the stopped
> group.

Right, we should be setting JOBCTL_STOP_PENDING for newly cloned tasks
if sigstop is in effect.

> And even if the new thread is auto-attached, we have the problems
> with JOBCTL_STOP_SIGMASK.
> 
> I do not want to discuss the "proper" solution here. I think the
> necessary changes are simple, but I do not think this is the 3.1
> material, and 3.1 needs some trivial fix.

Yeah, sure thing.

> What do you think about the patch below? I am going to send it to
> Linus unless you see something wrong.
> 
> And I'd like to explain why I prefer to add the (temporary) hack
> into __ptrace_unlink() instead of adding
> 
> 	if (unlikely(ptrace) && current->ptrace) {
> 		...
> 		child->jobctl = current->jobctl & JOBCTL_STOP_SIGMASK;
> 		...
> 	}
> 
> into ptrace_init_task(). I think that jobctl & JOBCTL_STOP_SIGMASK"
> should be cleanuped. Look, once we set JOBCTL_STOP_SIGMASK we never
> clear it. Yes, this doesn't really matter but this can hide an error,
> for example this can "fool" the WARN_ON(!signr) in do_jobctl_trap().
> Imho, at least SIGCONT should clear this part of ->jobctl. IOW, it
> should be non-zero only if/when it has effect.

I don't recall the details but there was somewhing convoluted about
clearing the signal number.  There's some non-obvious case where the
signr is used after what appears to be the apparent lifespan.  I
*think* we already talked about this but could be imagining things.
But, yeah, sure, if we can do it without complicating stuff, why not?

> That is why I do not want to change ptrace_init_task() until we
> decide what should we do with CLONE_THREAD && SIGNAL_STOP_STOPPED,
> to avoid the bogus (jobctl & JOBCTL_STOP_SIGMASK) != 0. IOW I prefer
> the change in __ptrace_unlink() to catch the other possible problems
> like this.

As long as it gets fixed properly in the next devel cycle, I don't
have any objection.

> Also, I'd like to defend 6634ae10
> "ptrace_init_task: initialize child->jobctl explicitly" which can
> be blamed for the 2nd problem. Afaics, this change is really needed
> and it fixes the bug. The changelog says "Currently this is harmless"
> but this is not right, dup_task_struct() can happen between SIGSTOP and
> SIGCONT, in this case the child can have the wrong JOBCTL_STOP_PENDING.
> 
> So, what do you think?

Looks good to me, provided proper fix is coming soon. :p

> -------------------------------------------------------------------------------
> [PATCH for 3.1] ptrace_unlink: ensure JOBCTL_STOP_SIGMASK is not zero
> 
> This is the temporary simple fix for 3.1, we need more changes in this
> area.
> 
> 1. do_signal_stop() assumes that the running untraced thread in the
>    stopped thread group is not possible. This was our goal but it is
>    not yet achieved: a stopped-but-resumed tracee can clone the running
>    thread which can initiate another group-stop.
> 
>    Remove WARN_ON_ONCE(!current->ptrace).
> 
> 2. A new thread always starts with ->jobctl = 0. If it is auto-attached
>    and this group is stopped, __ptrace_unlink() sets JOBCTL_STOP_PENDING
>    but JOBCTL_STOP_SIGMASK part is zero, this triggers WANR_ON(!signr)
>    in do_jobctl_trap() if another debugger attaches.
> 
>    Change __ptrace_unlink() to set the artificial SIGSTOP for report.
> 
>    Alternatively we could change ptrace_init_task() to copy signr from
>    current, but this means we can copy it for no reason and hide the
>    possible similar problems.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

 Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun