PTRACE_EVENT_SIGTRAP
This is used only in the UTRACE_SIGNAL_HANDLER case. That means after
tracehook_signal_handler(), which is where a signal handler has just
been set up.
For reference, in old ptrace, tracehook_signal_handler() is:
if (stepping)
ptrace_notify(SIGTRAP);
This means that when PTRACE_SINGLE{STEP,BLOCK},signr was used to
deliver the handled signal, we stop there in ptrace_notify. This is
in lieu of a proper single-step trap, with the theory that what you
meant to step to next was the first instruction of the signal
handler, rather than the second instruction of the handler.
The only purpose of PTRACE_EVENT_SIGTRAP is to distinguish this case
from PTRACE_EVENT_SIGNAL as it would be for a real SIGTRAP after a
normal instruction step. The only purposes of this distinction are to
make PTRACE_SETSIGINFO have no effect, and to make resumptions ignore
the data/signr argument. The former makes it consistent with other
cases in utrace-ptrace that replace ptrace_notify() calls in the old
ptrace, but that itself is a known nit incompatibility with the old
behavior, so it's superfluous at best. The latter is just intended
for pure bug-compatibility with the old behavior. Aside from these,
there are no other reasons not to have the UTRACE_SIGNAL_HANDLER case
result in PTRACE_EVENT_SIGNAL, with siginfo_t filled in to match what
the old ptrace_notify() does.
Is that all correct?
In short, PTRACE_EVENT_SIGTRAP provides bug compatibility for
PTRACE_SINGLESTEP,signr silently acting like PTRACE_SINGLESTEP,0 when
called at the stop just provoked by the last PTRACE_SINGLESTEP,signr.
Is that it?
I think this is a piece of bug compatibility that upstream will be
happy to have broken. That is, I bet most people don't realize at
all PTRACE_SINGLESTEP,signr will sometimes swallow a signal after what
appears to be a normal single-step stop. In fact, I suspect many
might say that this is worse than the previous behavior my addition of
this ptrace_notify() call (sometime before git so 2.6.12; Sep 2004,
not sure what version that was, maybe 2.6.9). That behavior was that
PTRACE_SINGLESTEP,signr would stop at the second instruction of the
signal handler and never the first--but that would be a real step
(like you now get after PTRACE_SINGLESTEP,0 when stopped in
ptrace_notify() at the first instruction of the handler). I'm quite
sure that at the time I never contemplated the signal-swallowing
implication of using ptrace_notify() here.
So, get rid of PTRACE_EVENT_SIGTRAP. Instead for the case of
UTRACE_SIGNAL_HANDLER when stepping, initialize *info to look
like a vanilla SIGTRAP and then fall into the default case for
real signals.
Does that all sound right to you, or did I miss something?
Thanks,
Roland
utrace-indirect branch
Please take a look at the patch below and tell me what you think. This
is the new(ish) utrace-indirect branch (not to be confused with what's
now old/utrace-indirect). I first made it a while ago, but I don't
recall if I ever mentioned it. This compiles and looks right, but I
have not done any testing at all.
Unlike the old code that freaked upstream reviewers all the way out,
this has very simple lifetime rules for struct utrace. Once allocated,
it lives until task_struct is freed. The utrace_task_alloc() logic
covers the only race (at first attach), and that seems pretty easy
to understand and be confident in.
For those of us who hack on utrace guts, this is much nicer. The struct
is private in utrace.c where it naturally belongs, and we don't have to
wait for the whole kernel to be recompiled every time we want to tweak
that struct.
For those of us who have to maintain long-term stable production kernels,
this is crucial. We need the freedom to change utrace implementation
details (and backport new utrace APIs) into our kernels in the future,
without perturbing the all-important task_struct layout. This is not a
concern that upstream ever cares about, but it is unavoidable in real life.
I am a bit concerned about letting the ugly utrace_struct.h way ever get
merged in, so that to fix it later we have to convince people all over
again about changing what works. But of course I am really not at all
sure whether even this innocuous version will upset the reviewers.
What do you think?
Thanks,
Roland
---
include/linux/init_task.h |1 -
include/linux/sched.h |3 +-
include/linux/tracehook.h | 12 ++
include/linux/utrace.h|9 ++---
include/linux/utrace_struct.h | 59 --
kernel/fork.c |1 +
kernel/utrace.c | 80 -
7 files changed, 89 insertions(+), 76 deletions(-)
diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index a598a6b..21a6f5d 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -177,7 +177,6 @@ extern struct cred init_cred;
[PIDTYPE_SID] = INIT_PID_LINK(PIDTYPE_SID),\
}, \
.dirties = INIT_PROP_LOCAL_SINGLE(dirties), \
- INIT_UTRACE(tsk)\
INIT_IDS\
INIT_PERF_EVENTS(tsk) \
INIT_TRACE_IRQFLAGS \
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 94ca5c5..8eeb46a 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -61,7 +61,6 @@ struct sched_param {
#include linux/errno.h
#include linux/nodemask.h
#include linux/mm_types.h
-#include linux/utrace_struct.h
#include asm/system.h
#include asm/page.h
@@ -1395,7 +1394,7 @@ struct task_struct {
seccomp_t seccomp;
#ifdef CONFIG_UTRACE
- struct utrace utrace;
+ struct utrace *utrace;
unsigned long utrace_flags;
#endif
diff --git a/include/linux/tracehook.h b/include/linux/tracehook.h
index 3caaf30..a01f8a9 100644
--- a/include/linux/tracehook.h
+++ b/include/linux/tracehook.h
@@ -609,6 +609,18 @@ static inline void tracehook_report_deat
utrace_report_death(task, death_cookie, group_dead, signal);
}
+/**
+ * tracehook_free_task - task_struct is being freed
+ * @task: dead struct task_struct being freed
+ *
+ * Called from free_task() when @task is no longer in use.
+ */
+static inline void tracehook_free_task(struct task_struct *task)
+{
+ if (task_utrace_struct(task))
+ utrace_free_task(task);
+}
+
#ifdef TIF_NOTIFY_RESUME
/**
* set_notify_resume - cause tracehook_notify_resume() to be called
diff --git a/include/linux/utrace.h b/include/linux/utrace.h
index f968792..6e8129b 100644
--- a/include/linux/utrace.h
+++ b/include/linux/utrace.h
@@ -92,6 +92,8 @@ enum utrace_events {
* tracehook.h doesn't need to #ifndef CONFIG_UTRACE them to
* avoid external references in case of unoptimized compilation.
*/
+void utrace_free_task(struct task_struct *)
+ __attribute__((weak));
bool utrace_interrupt_pending(void)
__attribute__((weak));
void utrace_resume(struct task_struct *, struct pt_regs *)
@@ -155,16 +157,13 @@ static inline unsigned long task_utrace_
static inline struct utrace *task_utrace_struct(struct task_struct *task)
{
- return task-utrace;
+ return task-utrace;
}
static inline void utrace_init_task(struct task_struct *task)
{
task-utrace_flags = 0;
- memset(task-utrace, 0, sizeof(task-utrace));
- INIT_LIST_HEAD(task-utrace.attached);
- INIT_LIST_HEAD(task-utrace.attaching);
- spin_lock_init(task-utrace.lock);
+
utrace-cleanup branch
I've made a new branch, utrace-cleanup. This forks from utrace-indirect and has: 26fefca utrace: sticky resume action 28b2774 utrace: remove -stopped field Those are the two changes we talked about during tangential ptrace discussions. Again, I have compiled these but not tested a lick. I'd just like to get some feedback. They both seem like happy clean-ups to me, making the code smaller and simpler. What do you think? Thanks, Roland
Re: utrace-indirect branch
On Wed, Oct 28, 2009 at 06:55:32PM -0700, Roland McGrath wrote:
Please take a look at the patch below and tell me what you think. This
is the new(ish) utrace-indirect branch (not to be confused with what's
now old/utrace-indirect). I first made it a while ago, but I don't
recall if I ever mentioned it. This compiles and looks right, but I
have not done any testing at all.
Unlike the old code that freaked upstream reviewers all the way out,
this has very simple lifetime rules for struct utrace. Once allocated,
it lives until task_struct is freed. The utrace_task_alloc() logic
covers the only race (at first attach), and that seems pretty easy
to understand and be confident in.
First glance, this looks much simpler and easier to understand compared
to the earlier rcu implementation.
...
+ if (!utrace) {
+ if (unlikely(!utrace_task_alloc(target)))
+ return ERR_PTR(-ENOMEM);
+ utrace = target-utrace;
utrace = task_utrace_struct(target);
+ }
+
engine = kmem_cache_alloc(utrace_engine_cachep, GFP_KERNEL);
if (unlikely(!engine))
return ERR_PTR(-ENOMEM);
Ananth
