Make PTRACE_SEIZE set ptrace options specified in data parameter

PATCH Superseded Denys Vlasenko 3 participants 4 messages Sep 7, 2011 – Sep 8, 2011 ptrace support

lore

Share & tools

lore

Denys Vlasenko

author

2011-09-07 04:47 UTC #0 lore raw

PATCH 1/1 Make PTRACE_SEIZE set ptrace options specified in data parameter

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 <[email protected]>

kernel/ptrace.c +10 -6

index 665ee93..ea83195 100644
@@ -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))

Oleg Nesterov

maintainer

2011-09-07 17:22 UTC #1 lore raw

Quoted 2 lines · Denys Vlasenko

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?

kernel/ptrace.c static int ptrace_attach(struct task_struct *task, long request, · line 238

Quoted 9 lines

kernel/ptrace.c +6 -2

 	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 = ...
	}

kernel/ptrace.c static int ptrace_attach(struct task_struct *task, long request, · line 272

Quoted 8 lines

kernel/ptrace.c +1 -1

@@ -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.

Denys Vlasenko

author

2011-09-07 21:40 UTC #2 lore raw

Quoted 13 lines · Oleg Nesterov

kernel/ptrace.c static int ptrace_attach(struct task_struct *task, long request, · line 238
Quoted 9 lines
kernel/ptrace.c +6 -2
 	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

Quoted 3 lines

	} else {
		flags = 0;
	}

Quoted 14 lines

kernel/ptrace.c static int ptrace_attach(struct task_struct *task, long request, · line 272
Quoted 8 lines
kernel/ptrace.c +1 -1
@@ -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.

Quoted 6 lines

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...

Signature · 1 line

vda

Tejun Heo

2011-09-08 00:28 UTC #3 lore raw

In reply to Oleg Nesterov

Hello,

Quoted 10 lines · Oleg Nesterov

Quoted 1 line · Denys Vlasenko
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.

Signature · 1 line

tejun