2009-04-29 12:33:31

by Oleg Nesterov

[permalink] [raw]
Subject: [PATCH] ptrace: do not use task->ptrace directly in core kernel

No functional changes.

- Nobody except ptrace.c & co should use ptrace flags directly, we have
task_ptrace() for that.

- No need to specially check PT_PTRACED, we must not have other PT_ bits
set without PT_PTRACED. And no need to know this flag exists.

Signed-off-by: Oleg Nesterov <[email protected]>
---

kernel/exit.c | 6 +++---
kernel/signal.c | 10 +++++-----
kernel/fork.c | 2 +-
3 files changed, 9 insertions(+), 9 deletions(-)

--- PTRACE/kernel/exit.c~CORE_FLAGS 2009-04-23 23:11:33.000000000 +0200
+++ PTRACE/kernel/exit.c 2009-04-29 12:46:15.000000000 +0200
@@ -757,7 +757,7 @@ static void reparent_thread(struct task_
p->exit_signal = SIGCHLD;

/* If it has exited notify the new parent about this child's death. */
- if (!p->ptrace &&
+ if (!task_ptrace(p) &&
p->exit_state == EXIT_ZOMBIE && thread_group_empty(p)) {
do_notify_parent(p, p->exit_signal);
if (task_detached(p)) {
@@ -782,7 +782,7 @@ static void forget_original_parent(struc
list_for_each_entry_safe(p, n, &father->children, sibling) {
p->real_parent = reaper;
if (p->parent == father) {
- BUG_ON(p->ptrace);
+ BUG_ON(task_ptrace(p));
p->parent = p->real_parent;
}
reparent_thread(father, p, &dead_children);
@@ -1478,7 +1478,7 @@ static int wait_consider_task(struct tas
*notask_error = ret;
}

- if (likely(!ptrace) && unlikely(p->ptrace)) {
+ if (likely(!ptrace) && unlikely(task_ptrace(p))) {
/*
* This child is hidden by ptrace.
* We aren't allowed to see it now, but eventually we will.
--- PTRACE/kernel/signal.c~CORE_FLAGS 2009-04-06 00:03:42.000000000 +0200
+++ PTRACE/kernel/signal.c 2009-04-29 13:21:47.000000000 +0200
@@ -1402,7 +1402,7 @@ int do_notify_parent(struct task_struct
/* do_notify_parent_cldstop should have been called instead. */
BUG_ON(task_is_stopped_or_traced(tsk));

- BUG_ON(!tsk->ptrace &&
+ BUG_ON(!task_ptrace(tsk) &&
(tsk->group_leader != tsk || !thread_group_empty(tsk)));

info.si_signo = sig;
@@ -1441,7 +1441,7 @@ int do_notify_parent(struct task_struct

psig = tsk->parent->sighand;
spin_lock_irqsave(&psig->siglock, flags);
- if (!tsk->ptrace && sig == SIGCHLD &&
+ if (!task_ptrace(tsk) && sig == SIGCHLD &&
(psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN ||
(psig->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDWAIT))) {
/*
@@ -1478,7 +1478,7 @@ static void do_notify_parent_cldstop(str
struct task_struct *parent;
struct sighand_struct *sighand;

- if (tsk->ptrace & PT_PTRACED)
+ if (task_ptrace(tsk))
parent = tsk->parent;
else {
tsk = tsk->group_leader;
@@ -1527,7 +1527,7 @@ static void do_notify_parent_cldstop(str

static inline int may_ptrace_stop(void)
{
- if (!likely(current->ptrace & PT_PTRACED))
+ if (!likely(task_ptrace(current)))
return 0;
/*
* Are we in the middle of do_coredump?
@@ -1745,7 +1745,7 @@ static int do_signal_stop(int signr)
static int ptrace_signal(int signr, siginfo_t *info,
struct pt_regs *regs, void *cookie)
{
- if (!(current->ptrace & PT_PTRACED))
+ if (!task_ptrace(current))
return signr;

ptrace_signal_deliver(regs, cookie);
--- PTRACE/kernel/fork.c~CORE_FLAGS 2009-04-27 21:38:57.000000000 +0200
+++ PTRACE/kernel/fork.c 2009-04-29 13:25:29.000000000 +0200
@@ -1088,7 +1088,7 @@ static struct task_struct *copy_process(
#ifdef CONFIG_DEBUG_MUTEXES
p->blocked_on = NULL; /* not blocked yet */
#endif
- if (unlikely(current->ptrace))
+ if (unlikely(task_ptrace(current)))
ptrace_fork(p, clone_flags);

/* Perform scheduler related setup. Assign this task to a CPU. */


2009-04-30 20:29:28

by Roland McGrath

[permalink] [raw]
Subject: Re: [PATCH] ptrace: do not use task->ptrace directly in core kernel

That is fine, but doesn't buy much. i.e., we will be changing these again
before too long anyway I imagine.

I added task_ptrace() just for tracehook.h use, really. There it drives
the event hooks. Those uses are directly obsoleted by using another event
hooking mechanism such as utrace. That applies to ptrace_signal() too.
But the other uses will be replaced by something different later, not just
go away.

The BUG_ON cases might as well just go away, probably.

The exit.c cases might be clearer if we give them a (trivial) local helper
with a more topical name like task_wait_inhibited().

ptrace_fork() is a wrapper that just calls arch_ptrace_fork(), which itself
is an empty macro on most configurations. I think we might as well just
make ptrace_fork() an inline in linux/ptrace.h and put the test inside it.
(Thus any future changes touch ptrace.h and not fork.c.)


Thanks,
Roland