Re: ptrace() hangs on attempt to seize/attach stopped & frozen task

[Tejun Heo <tj@kernel.org>]

Hey, Oleg.

On Tue, Nov 17, 2015 at 08:34:19PM +0100, Oleg Nesterov wrote:
> On 11/16, Tejun Heo wrote:
> >
> > *** WARNING: THE ATTACHED DOCUMENT(S) CONTAIN MACROS ***
> > *** MACROS MAY CONTAIN MALICIOUS CODE ***
> > *** Open only if you can verify and trust the sender ***
> > *** Please contact infosec@redhat.com if you have questions or concerns **
> 
> Hmm, infosec@redhat.com doesn't like you. But I dared to open and nothing
> happened so far. although perhaps you already own my machine.

lol no idea what's going on there but dude you gotta clean up the
browsing history.

> > Hmmm... It's nasty tho.  We're breaking a guaranteed userland behavior
> 
> Perhaps you are right, but I am wondering if it was ever guaranteed.
> 
> What actually annoys me is that now I am almost sure that it was me
> who asked you to hide this from user-space, and today I see no reason
> for this hack.
> 
> > I'd be a lot more comfortable stating
> > that cgroup freezer is currently broken rather than diddling with
> > subtle ptrace semantics.
> 
> OK, lets keep this JOBCTL_TRAPPING_BIT.
> 
> But still I would like to know what Pedro thinks...
> 
> Anyway, wait_on_bit(TASK_UNINTERRUPTIBLE) doesn't look good. Do you
> see any problem with the change below? Yes, the comment is not clear,
> it should be updated, the tracee can clear this bit too.
> 
> And perhaps we can change get_task_state() until freezer gets another state,
> 
> 	--- x/fs/proc/array.c
> 	+++ x/fs/proc/array.c
> 	@@ -126,6 +126,9 @@ static inline const char *get_task_state
> 	 {
> 		unsigned int state = (tsk->state | tsk->exit_state) & TASK_REPORT;
> 	 
> 	+	if (tsk->flags & PF_FROZEN)
> 	+		return "D (frozen)";
> 	+
> 		BUILD_BUG_ON(1 + ilog2(TASK_REPORT) != ARRAY_SIZE(task_state_array)-1);
> 	 
> 		return task_state_array[fls(state)];

Hmm... the only nit is that we'll eventually want to share "T
(stopped)" or do "T (frozen)" and switching down the road could be a
bit confusing.  It shouldn't be a big deal tho.  I think I'm mostly
reluctant to accomodate the broken behavior of cgroup freezer.

> --- x/kernel/ptrace.c
> +++ x/kernel/ptrace.c
> @@ -364,8 +364,13 @@ unlock_creds:
>  	mutex_unlock(&task->signal->cred_guard_mutex);
>  out:
>  	if (!retval) {
> -		wait_on_bit(&task->jobctl, JOBCTL_TRAPPING_BIT,
> -			    TASK_UNINTERRUPTIBLE);
> +		if (wait_on_bit(&task->jobctl, JOBCTL_TRAPPING_BIT,
> +				TASK_KILLABLE))
> +			/*
> +			 * We will clear JOBCTL_TRAPPING in __ptrace_unlink(),
> +			 * until then nobody can trace this task anyway.
> +			 */
> +			retval = -EINTR;

Yeah, this looks good to me.

Thanks.

-- 
tejun