* [PATCH] Simplify PTRACE_foo constants and PTRACE_SETOPTIONS code
@ 2011-09-07 21:36 Denys Vlasenko
2011-09-08 0:06 ` Tejun Heo
0 siblings, 1 reply; 4+ messages in thread
From: Denys Vlasenko @ 2011-09-07 21:36 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Tejun Heo, linux-kernel, Denys Vlasenko
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 <dvlasenk@redhat.com>
diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index 800f113..0911100 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -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)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 9de3ecf..0316200 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -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)
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Simplify PTRACE_foo constants and PTRACE_SETOPTIONS code
2011-09-07 21:36 [PATCH] Simplify PTRACE_foo constants and PTRACE_SETOPTIONS code Denys Vlasenko
@ 2011-09-08 0:06 ` Tejun Heo
2011-09-08 12:13 ` Denys Vlasenko
0 siblings, 1 reply; 4+ messages in thread
From: Tejun Heo @ 2011-09-08 0:06 UTC (permalink / raw)
To: Denys Vlasenko; +Cc: Oleg Nesterov, linux-kernel
Hello,
Mostly looks good to me. Just a small nit.
On Wed, Sep 07, 2011 at 11:36:37PM +0200, Denys Vlasenko wrote:
> 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.
> + 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 <tj@kernel.org>
Thank you.
--
tejun
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Simplify PTRACE_foo constants and PTRACE_SETOPTIONS code
2011-09-08 0:06 ` Tejun Heo
@ 2011-09-08 12:13 ` Denys Vlasenko
2011-09-08 18:37 ` Oleg Nesterov
0 siblings, 1 reply; 4+ messages in thread
From: Denys Vlasenko @ 2011-09-08 12:13 UTC (permalink / raw)
To: Tejun Heo; +Cc: Oleg Nesterov, linux-kernel
On Thu, Sep 8, 2011 at 2:06 AM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> Mostly looks good to me. Just a small nit.
>
> On Wed, Sep 07, 2011 at 11:36:37PM +0200, Denys Vlasenko wrote:
>> 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.
>> + 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).
--
vda
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Simplify PTRACE_foo constants and PTRACE_SETOPTIONS code
2011-09-08 12:13 ` Denys Vlasenko
@ 2011-09-08 18:37 ` Oleg Nesterov
0 siblings, 0 replies; 4+ messages in thread
From: Oleg Nesterov @ 2011-09-08 18:37 UTC (permalink / raw)
To: Denys Vlasenko; +Cc: Tejun Heo, linux-kernel
On 09/08, Denys Vlasenko wrote:
>
> On Thu, Sep 8, 2011 at 2:06 AM, Tejun Heo <tj@kernel.org> wrote:
> > 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.
> 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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-09-08 23:34 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-07 21:36 [PATCH] Simplify PTRACE_foo constants and PTRACE_SETOPTIONS code Denys Vlasenko
2011-09-08 0:06 ` Tejun Heo
2011-09-08 12:13 ` Denys Vlasenko
2011-09-08 18:37 ` Oleg Nesterov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.