Simplify PTRACE_foo constants and PTRACE_SETOPTIONS code

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

lore

Share & tools

lore

Denys Vlasenko

author

2011-09-07 21:36 UTC #0 lore raw

PATCH 1/1 Simplify PTRACE_foo constants and PTRACE_SETOPTIONS code

Simplify PTRACE_foo constants and PTRACE_SETOPTIONS code

Exchange PT_TRACESYSGOOD and PT_PTRACE_CAP bit positions, which makes PT_option bits contiguous and therefore makes code in ptrace_setoptions() much simpler.

Every PTRACE_O_TRACEevent is defined to (1 << PTRACE_EVENT_event) instead of using explicit numeric constants, to ensure we don't mess up relationship between bit positions and event ids.

PT_EVENT_FLAG_SHIFT was not particularly useful, PT_OPT_FLAG_SHIFT with value of PT_EVENT_FLAG_SHIFT-1 is easier to use.

PT_TRACE_MASK constant is nuked, the only its use is replaced by (PTRACE_O_MASK << PT_OPT_FLAG_SHIFT).

Clearly buggy behavior of PTRACE_SETOPTIONS with unknown bits in flags is preseved. The comment about bug is added.

Signed-off-by Denys Vlasenko <[email protected]>

include/linux/ptrace.h +15 ~~-18~~

index 800f113..0911100 100644
@@ -54,17 +54,6 @@
 /* flags in @data for PTRACE_SEIZE */
 #define PTRACE_SEIZE_DEVEL	0x80000000 /* temp flag for development */
 
-/* options set using PTRACE_SETOPTIONS */
-#define PTRACE_O_TRACESYSGOOD	0x00000001
-#define PTRACE_O_TRACEFORK	0x00000002
-#define PTRACE_O_TRACEVFORK	0x00000004
-#define PTRACE_O_TRACECLONE	0x00000008
-#define PTRACE_O_TRACEEXEC	0x00000010
-#define PTRACE_O_TRACEVFORKDONE	0x00000020
-#define PTRACE_O_TRACEEXIT	0x00000040
-
-#define PTRACE_O_MASK		0x0000007f
-
 /* Wait extended result codes for the above trace options.  */
 #define PTRACE_EVENT_FORK	1
 #define PTRACE_EVENT_VFORK	2

@@ -74,6 +63,17 @@
 #define PTRACE_EVENT_EXIT	6
 #define PTRACE_EVENT_STOP	7
 
+/* options set using PTRACE_SETOPTIONS */
+#define PTRACE_O_TRACESYSGOOD	1
+#define PTRACE_O_TRACEFORK	(1 << PTRACE_EVENT_FORK)
+#define PTRACE_O_TRACEVFORK	(1 << PTRACE_EVENT_VFORK)
+#define PTRACE_O_TRACECLONE	(1 << PTRACE_EVENT_CLONE)
+#define PTRACE_O_TRACEEXEC	(1 << PTRACE_EVENT_EXEC)
+#define PTRACE_O_TRACEVFORKDONE	(1 << PTRACE_EVENT_VFORK_DONE)
+#define PTRACE_O_TRACEEXIT	(1 << PTRACE_EVENT_EXIT)
+
+#define PTRACE_O_MASK		0x0000007f
+
 #include <asm/ptrace.h>
 
 #ifdef __KERNEL__

@@ -88,13 +88,12 @@
 #define PT_SEIZED	0x00010000	/* SEIZE used, enable new behavior */
 #define PT_PTRACED	0x00000001
 #define PT_DTRACE	0x00000002	/* delayed trace (used on m68k, i386) */
-#define PT_TRACESYSGOOD	0x00000004
-#define PT_PTRACE_CAP	0x00000008	/* ptracer can follow suid-exec */
+#define PT_PTRACE_CAP	0x00000004	/* ptracer can follow suid-exec */
 
+#define PT_OPT_FLAG_SHIFT	3
 /* PT_TRACE_* event enable flags */
-#define PT_EVENT_FLAG_SHIFT	4
-#define PT_EVENT_FLAG(event)	(1 << (PT_EVENT_FLAG_SHIFT + (event) - 1))
-
+#define PT_EVENT_FLAG(event)	(1 << (PT_OPT_FLAG_SHIFT + (event)))
+#define PT_TRACESYSGOOD		PT_EVENT_FLAG(0)
 #define PT_TRACE_FORK		PT_EVENT_FLAG(PTRACE_EVENT_FORK)
 #define PT_TRACE_VFORK		PT_EVENT_FLAG(PTRACE_EVENT_VFORK)
 #define PT_TRACE_CLONE		PT_EVENT_FLAG(PTRACE_EVENT_CLONE)

@@ -102,8 +101,6 @@
 #define PT_TRACE_VFORK_DONE	PT_EVENT_FLAG(PTRACE_EVENT_VFORK_DONE)
 #define PT_TRACE_EXIT		PT_EVENT_FLAG(PTRACE_EVENT_EXIT)
 
-#define PT_TRACE_MASK	0x000003f4
-
 /* single stepping state bits (used on ARM and PA-RISC) */
 #define PT_SINGLESTEP_BIT	31
 #define PT_SINGLESTEP		(1<<PT_SINGLESTEP_BIT)

kernel/ptrace.c +12 ~~-22~~

index 9de3ecf..0316200 100644
@@ -243,7 +243,7 @@ static int ptrace_attach(struct task_struct *task, long request,
 
 	/*
 	 * Protect exec's credential calculations against our interference;
-	 * interference; SUID, SGID and LSM creds get determined differently
+	 * SUID, SGID and LSM creds get determined differently
 	 * under ptrace.
 	 */
 	retval = -ERESTARTNOINTR;

@@ -509,30 +509,20 @@ int ptrace_writedata(struct task_struct *tsk, char __user *src, unsigned long ds
 
 static int ptrace_setoptions(struct task_struct *child, unsigned long data)
 {
-	child->ptrace &= ~PT_TRACE_MASK;
+	unsigned flags;
 
-	if (data & PTRACE_O_TRACESYSGOOD)
-		child->ptrace |= PT_TRACESYSGOOD;
-
-	if (data & PTRACE_O_TRACEFORK)
-		child->ptrace |= PT_TRACE_FORK;
-
-	if (data & PTRACE_O_TRACEVFORK)
-		child->ptrace |= PT_TRACE_VFORK;
-
-	if (data & PTRACE_O_TRACECLONE)
-		child->ptrace |= PT_TRACE_CLONE;
-
-	if (data & PTRACE_O_TRACEEXEC)
-		child->ptrace |= PT_TRACE_EXEC;
-
-	if (data & PTRACE_O_TRACEVFORKDONE)
-		child->ptrace |= PT_TRACE_VFORK_DONE;
+	if (data & ~(long)PTRACE_O_MASK) {
+		child->ptrace &= ~PT_TRACE_MASK; /* historical bug */
+		return -EINVAL;
+	}
 
-	if (data & PTRACE_O_TRACEEXIT)
-		child->ptrace |= PT_TRACE_EXIT;
+	/* Avoid intermediate state when all opts are cleared */
+	flags = child->ptrace;
+	flags &= ~(PTRACE_O_MASK << PT_OPT_FLAG_SHIFT);
+	flags |= (data << PT_OPT_FLAG_SHIFT);
+	child->ptrace = flags;
 
-	return (data & ~PTRACE_O_MASK) ? -EINVAL : 0;
+	return 0;
 }
 
 static int ptrace_getsiginfo(struct task_struct *child, siginfo_t *info)

Tejun Heo

Acked-by on v1 1/1

2011-09-08 00:06 UTC #1 lore raw

Hello,

Mostly looks good to me. Just a small nit.

kernel/ptrace.c int ptrace_writedata(struct task_struct *tsk, char __user *src, unsigned long ds · line 514

Quoted 5 lines · Denys Vlasenko

static int ptrace_setoptions(struct task_struct *child, unsigned long data) {
kernel/ptrace.c +2 -0
+	unsigned flags;
 
+	if (data & ~(long)PTRACE_O_MASK) {

Can we use (unsigned long) here? Signed extensions have a nasty way of sneaking up on you.

kernel/ptrace.c int ptrace_writedata(struct task_struct *tsk, char __user *src, unsigned long ds · line 515

Quoted 1 line

kernel/ptrace.c +1 -0

+		child->ptrace &= ~PT_TRACE_MASK; /* historical bug */

And it would be great if it explains what the historical bug was and what the intended behavior is.

Other than that,

Acked-by Tejun Heo <[email protected]>

Thank you.

Signature · 1 line

tejun

Denys Vlasenko

author

2011-09-08 12:13 UTC #2 lore raw

Quoted 5 lines · Tejun Heo

Hello,
Mostly looks good to me. Just a small nit.
On Wed, Sep 07, 2011 at 11:36:37PM +0200, Denys Vlasenko wrote:

Quoted 8 lines

Quoted 5 lines
static int ptrace_setoptions(struct task_struct *child, unsigned long data) {
+ unsigned flags;
+ if (data & ~(long)PTRACE_O_MASK) {
Can we use (unsigned long) here? Signed extensions have a nasty way of sneaking up on you.

Ok.

Quoted 4 lines

Quoted 1 line
+ child->ptrace &= ~PT_TRACE_MASK; /* historical bug */
And it would be great if it explains what the historical bug was and what the intended behavior is.

I'd rather nuke the bug in a follow-up patch (by deleting this line).

Signature · 1 line

vda

Oleg Nesterov

maintainer

2011-09-08 23:34 UTC #3 lore raw

Quoted 4 lines · Denys Vlasenko

Quoted 2 lines · Tejun Heo
And it would be great if it explains what the historical bug was and what the intended behavior is.

ptrace_setoptions() always changes ->ptrace, even if it returns -EINVAL later.

Quoted 1 line

I'd rather nuke the bug in a follow-up patch (by deleting this line).

Denys, I think it would be much better to send 1/2 which fixes this oddity.

And note that this patch is wrong, I'll comment v3.

Oleg.