[PATCH] Make PTRACE_SEIZE set ptrace options specified in data parameter

[PATCH] Make PTRACE_SEIZE set ptrace options specified in data parameter

[Denys Vlasenko]

Make PTRACE_SEIZE set ptrace options specified in data parameter
    
This can be used to close a few corner cases in strace where we get
unwanted behavior after attach, but before we have a chance
to set options (the notorious post-execve SIGTRAP comes to mind),
and removes the need to track "did we set opts for this task?" state
in strace internals.
    
Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 665ee93..ea83195 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -219,19 +219,23 @@ static int ptrace_attach(struct task_struct *task, long request,
 
 	/*
 	 * SEIZE will enable new ptrace behaviors which will be implemented
-	 * gradually.  SEIZE_DEVEL is used to prevent applications
+	 * gradually.  SEIZE_DEVEL bit is used to prevent applications
 	 * expecting full SEIZE behaviors trapping on kernel commits which
 	 * are still in the process of implementing them.
 	 *
 	 * Only test programs for new ptrace behaviors being implemented
 	 * should set SEIZE_DEVEL.  If unset, SEIZE will fail with -EIO.
 	 *
-	 * Once SEIZE behaviors are completely implemented, this flag and
-	 * the following test will be removed.
+	 * Once SEIZE behaviors are completely implemented, this flag
+	 * will be removed.
 	 */
 	retval = -EIO;
-	if (seize && !(flags & PTRACE_SEIZE_DEVEL))
-		goto out;
+	if (seize) {
+		if ((flags & ~(long)PTRACE_O_MASK) != PTRACE_SEIZE_DEVEL)
+			goto out;
+		flags &= ~PTRACE_SEIZE_DEVEL;
+	} else
+		flags = 0;
 
 	audit_ptrace(task);
 
@@ -263,7 +267,7 @@ static int ptrace_attach(struct task_struct *task, long request,
 	if (task->ptrace)
 		goto unlock_tasklist;
 
-	task->ptrace = PT_PTRACED;
+	task->ptrace = PT_PTRACED | (flags << PT_OPT_FLAG_SHIFT);
 	if (seize)
 		task->ptrace |= PT_SEIZED;
 	if (task_ns_capable(task, CAP_SYS_PTRACE))

Re: [PATCH] Make PTRACE_SEIZE set ptrace options specified in data parameter

[Oleg Nesterov]

On 09/07, Denys Vlasenko wrote:
>
> Make PTRACE_SEIZE set ptrace options specified in data parameter

Personally I think this makes sense, and in fact I suggested this
from the very beginning.

Oh. But since we are goin to establish the API rules, this should
be discussed.

IIRC, Tejun disliked this idea. Tejun?

>  	retval = -EIO;
> -	if (seize && !(flags & PTRACE_SEIZE_DEVEL))
> -		goto out;
> +	if (seize) {
> +		if ((flags & ~(long)PTRACE_O_MASK) != PTRACE_SEIZE_DEVEL)
> +			goto out;
> +		flags &= ~PTRACE_SEIZE_DEVEL;
> +	} else
> +		flags = 0;

Personally I do not care, but this is against the coding-style rules. This
should be

	} else {
		flags = 0;
	}

Or, better,

	flags = 0;
	if (seize) {
		flags = ...
	}


> @@ -263,7 +267,7 @@ static int ptrace_attach(struct task_struct *task, long request,
>  	if (task->ptrace)
>  		goto unlock_tasklist;
>
> -	task->ptrace = PT_PTRACED;
> +	task->ptrace = PT_PTRACED | (flags << PT_OPT_FLAG_SHIFT);
>  	if (seize)
>  		task->ptrace |= PT_SEIZED;

Hmm. Tejun, Denys, this doesn't look exactly right.

I already thought about this before, but somehow I convinced myself
this is fine.

I think we should set both PT_PTRACED | PT_SEIZED "atomically", at
once. Otherwise, say, the tracee can do do_jobctl_trap() in between,
no? Nothing really bad can happen, but we shouldn't lose EVENT_STOP
code.

IOW, I think we need the small fix before this patch.

Oleg.

Re: [PATCH] Make PTRACE_SEIZE set ptrace options specified in data parameter

[Denys Vlasenko]

On Wednesday 07 September 2011 19:02, Oleg Nesterov wrote:
> >  	retval = -EIO;
> > -	if (seize && !(flags & PTRACE_SEIZE_DEVEL))
> > -		goto out;
> > +	if (seize) {
> > +		if ((flags & ~(long)PTRACE_O_MASK) != PTRACE_SEIZE_DEVEL)
> > +			goto out;
> > +		flags &= ~PTRACE_SEIZE_DEVEL;
> > +	} else
> > +		flags = 0;
> 
> Personally I do not care, but this is against the coding-style rules. This
> should be
> 
> 	} else {
> 		flags = 0;
> 	}

Ok


> > @@ -263,7 +267,7 @@ static int ptrace_attach(struct task_struct *task, long request,
> >  	if (task->ptrace)
> >  		goto unlock_tasklist;
> >
> > -	task->ptrace = PT_PTRACED;
> > +	task->ptrace = PT_PTRACED | (flags << PT_OPT_FLAG_SHIFT);
> >  	if (seize)
> >  		task->ptrace |= PT_SEIZED;
> 
> Hmm. Tejun, Denys, this doesn't look exactly right.
> 
> I already thought about this before, but somehow I convinced myself
> this is fine.
> 
> I think we should set both PT_PTRACED | PT_SEIZED "atomically", at
> once. Otherwise, say, the tracee can do do_jobctl_trap() in between,
> no? Nothing really bad can happen, but we shouldn't lose EVENT_STOP
> code.
> 
> IOW, I think we need the small fix before this patch.

I think the best is to just do one and only assignment to task->ptrace.
Even before patch, we can touch it three times...

Sending patch v2...

-- 
vda

Re: [PATCH] Make PTRACE_SEIZE set ptrace options specified in data parameter

[Tejun Heo]

Hello,

On Wed, Sep 07, 2011 at 07:02:12PM +0200, Oleg Nesterov wrote:
> On 09/07, Denys Vlasenko wrote:
> > Make PTRACE_SEIZE set ptrace options specified in data parameter
> 
> Personally I think this makes sense, and in fact I suggested this
> from the very beginning.
> 
> Oh. But since we are goin to establish the API rules, this should
> be discussed.
> 
> IIRC, Tejun disliked this idea. Tejun?

I wasn't sure whether it was necessary but TRACEEXEC needs to be set
atomically and it probably is better to expose flag setting than
implicitly setting it on SEIZE, so no objection from me.

Will comment on the newer version of the patch.

Thanks.

-- 
tejun