Re: [PATCH] utrace: finish_report() must never set -resume = UTRACE_STOP
Forgot to mention, your tree lacks these patches we sent upstream: Right. I'll merge these into some requirements branch (or maybe just the existing tracehook branch) and merge that into utrace. Thanks, Roland
Re: [PATCH] utrace: utrace_attach_task() forgets to return when -utrace == NULL
Now, if we race with another task doing utrace_task_alloc() and see -utrace != NULL, why should we see the correctly initialized *utrace? utrace_task_alloc() needs wmb(), and the code like above read_barrier_depends(). Ok. Please review what I put in (esp. commit c575558) and send patches relative to the latest tree if that's not right. Perhaps it is overkill to do read_barrier_depends() in task_utrace_struct(). But AFAICT if we don't actually need it all those places, that is only because of some less-obvious barrier effect with checks on -utrace_flags or something. Do you think it's not really needed in all those places we extract the pointer before spin_lock et al? Thanks, Roland
[HACK] utrace: fix utrace_resume()-finish_resume_report() logic
In short, it is just wrong to call finish_resume_report() in utrace_resume()
without reporting loop, because utrace never clears TIF_NOTIFY_RESUME. It is
very possible we enter utrace_resume() with utrace-resume == UTRACE_RESUME,
in this case finish_resume_report() does user_disable_single_step(). And if
TIF_SINGLESTEP was previously set by utrace_get_signal() which noticed
-resume UTRACE_RESUME we lost: engine can't re-assert the stepping.
IOW. Just one example. The tracee reports the signal and sleeps in
utrace_get_signal()-finish_resume_report().
Suppose that the tracee has TIF_NOTIFY_RESUME bit set. A lot of reasons
why this can happen.
The tracer wakes up the tracee via utrace_control(UTRACE_SINGLESTEP).
The tracee resumes, calls utrace_get_signal_again(), notices
utrace-resume UTRACE_RESUME, resets utrace-resume, and does
finish_resume_report()-enable_step() correctly.
But, since TIF_NOTIFY_RESUME is set, the tracee will call utrace_resume()
before return to user-mode, and the next finish_resume_report() from
utrace_resume() clears TIF_SINGLESTEP.
Of course, this is just the temporary hack, to do the testing right now.
I still do not understand the new code in details, need to read it
carefully but didn't have the time yet :/
---
kernel/utrace.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- UTRACE-PTRACE/kernel/utrace.c~UTRACE_HACK_RESUME2009-11-16
17:02:11.0 +0100
+++ UTRACE-PTRACE/kernel/utrace.c 2009-11-16 17:28:10.0 +0100
@@ -1871,7 +1871,7 @@ void utrace_resume(struct task_struct *t
*/
report.action = start_report(utrace);
- if (report.action == UTRACE_REPORT
+ if (1 //report.action == UTRACE_REPORT
likely(task-utrace_flags UTRACE_EVENT(QUIESCE))) {
/*
* Do a simple reporting pass, with no specific
[PATCH 133] stepping, accommodate to utrace-cleanup changes
This needs more comment, I'll try to add them in a separate patch.
With the recent changes in utrace API utrace_control(UTRACE_SINGLESTEP)
postpones enable_step() to utrace_resume() stage. The tracee can pass
tracehook_report_syscall_exit() without TIF_SINGLESTEP, this breaks
the send_sigtrap() logic.
Change ptrace_resume_action() to set UTRACE_EVENT(SYSCALL_EXIT) in
PTRACE_SINGLESTEP/PTRACE_SINGLEBLOCK case, change report_syscall_exit()
to check ctx-resume to detect this case. To synthesize the trap we
set ctx-signr = SIGTRAP and provoke UTRACE_SIGNAL_REPORT, like
ptrace_resume() does.
Roland, we already discussed this a bit and I agree, this is not optimal.
We need some changes in utrace, but currently it has a lot of problems
and it is not clear yet what utrace should do. Let's fix the code now,
once we find a better solution we can revert this change.
With this patch make check passes all tests again (to clarify, except
some tests which upstream doesn't pass too), including this one:
#include stdio.h
#include unistd.h
#include stdlib.h
#include signal.h
#include errno.h
#include sys/wait.h
#include sys/ptrace.h
#include assert.h
#include sys/user.h
#include sys/debugreg.h
#include sys/syscall.h
#define WEVENT(s) ((s 0xFF) 16)
static int verbose;
#define d_printf(fmt, ...) do { if (verbose) printf(fmt,
##__VA_ARGS__); } while (0)
static struct user_regs_struct regs;
static void resume(int pid, int req, int ck_stat)
{
int stat;
assert(0 == ptrace(req, pid, 0, 0));
assert(waitpid(pid, stat, __WALL) == pid);
//d_printf(=== %06X\n %06X\n, ck_stat, stat);
assert(stat == ck_stat);
assert(0 == ptrace(PTRACE_GETREGS, pid, NULL, regs));
}
int main(int argc, const char *argv[])
{
int pid, child, stat;
long rip, nxt_rip;
if (getpid() == __NR_getppid) {
printf(sorry, restart\n);
return 0;
}
verbose = argc 1;
pid = fork();
if (!pid) {
assert(0 == ptrace(PTRACE_TRACEME, 0,0,0));
kill(getpid(), SIGSTOP);
// 1: SYSCALL + SYSCALL + STEP
getppid();
// 2: SYSCALL + STEP
getppid();
// 3: STEP
getppid();
// 4: SYSCALL + STEP
if (!fork()) exit(73);
// STEPs only
if (!fork()) exit(73);
assert(0);
}
assert(wait(stat) == pid);
assert(WIFSTOPPED(stat) WSTOPSIG(stat) == SIGSTOP);
assert(0 == ptrace(PTRACE_SETOPTIONS, pid, 0,
PTRACE_O_TRACESYSGOOD |
PTRACE_O_TRACEFORK));
//-
d_printf(1: syscall enter\n);
resume(pid, PTRACE_SYSCALL, 0x857F);
assert(regs.orig_rax == __NR_getppid);
assert(regs.rax == -ENOSYS);
rip = regs.rip;
d_printf(1: syscall leave\n);
resume(pid, PTRACE_SYSCALL, 0x857F);
assert(regs.orig_rax == __NR_getppid);
assert(regs.rax == getpid());
assert(regs.rip == rip);
d_printf(1: singlestep\n);
resume(pid, PTRACE_SINGLESTEP, 0x57F);
assert((long)regs.orig_rax 0);
assert(regs.rax == getpid());
assert(regs.rip != rip);
d_printf(1: singlestep\n);
resume(pid, PTRACE_SINGLESTEP, 0x57F);
assert(regs.rip != rip);
//
d_printf(2: stop before syscall insn\n);
do {
resume(pid, PTRACE_SINGLESTEP, 0x57F);
} while (regs.rip != rip - 2);
assert(regs.rax == __NR_getppid);
d_printf(2: syscall enter\n);
resume(pid, PTRACE_SYSCALL, 0x857F);
assert(regs.orig_rax == __NR_getppid);
assert(regs.rax == -ENOSYS);
assert(regs.rip == rip);
d_printf(2: singlestep\n);
resume(pid, PTRACE_SINGLESTEP, 0x57F);
assert(regs.orig_rax == __NR_getppid);
assert(regs.rax == getpid());
assert(regs.rip == rip);
d_printf(2: singlestep\n);
resume(pid, PTRACE_SINGLESTEP,
Re: [PATCH 133] stepping, accommodate to utrace-cleanup changes
Just in case, forgot to clarify... On 11/16, Oleg Nesterov wrote: With this patch make check passes all tests again (to clarify, except some tests which upstream doesn't pass too), including this one: with this patch + [HACK] utrace: fix utrace_resume()-finish_resume_report() logic And I didn't check make xcheck, I guess it still crashes the kernel. Oleg.
Re: [PATCH] utrace: utrace_attach_task() forgets to return when -utrace == NULL
On 11/16, Roland McGrath wrote:
Now, if we race with another task doing utrace_task_alloc() and see
-utrace != NULL, why should we see the correctly initialized *utrace?
utrace_task_alloc() needs wmb(), and the code like above
read_barrier_depends().
Ok. Please review what I put in (esp. commit c575558)
Yes, I think this is correct.
Just in case fyi, I cooked the almost identical patch yesterday, but
it didn't help: make xcheck crashes. Not that I really expected it can
help on x86.
Perhaps it is overkill to
do read_barrier_depends() in task_utrace_struct().
Yes, perhaps. But this is safer.
Do you think it's not really needed in all those places we extract the
pointer before spin_lock et al?
Not sure I understand exactly... Yes, sometimes we know we don't need
read_barrier_depends(). For example,
@@ -302,7 +309,7 @@ struct utrace_engine *utrace_attach_task(
if (!utrace) {
if (unlikely(!utrace_task_alloc(target)))
return ERR_PTR(-ENOMEM);
- utrace = target-utrace;
+ utrace = task_utrace_struct(target);
}
I think this change is not necessary (but imho good anyway), this path
must take task_lock() and thus it must always see the correct *utrace.
But read_barrier_depends() does nothig except on alpha, why should we
care?
This reminds me, utrace_interrupt_pending() and similar code needs rmb()
or task-utrace != NULL check.
Oleg.
Re: [HACK] utrace: fix utrace_resume()-finish_resume_report() logic
In short, it is just wrong to call finish_resume_report() in utrace_resume()
without reporting loop, because utrace never clears TIF_NOTIFY_RESUME.
It's not supposed to. The arch code clears TIF_NOTIFY_RESUME before
calling tracehook_notify_resume(). This implies that utrace must keep its
own independent bookkeeping sufficient to know what to do when there is a
spurious call to utrace_resume().
It is
very possible we enter utrace_resume() with utrace-resume == UTRACE_RESUME,
Let's figure out exactly when this can happen. Perhaps we need more state
bits in some form. But perhaps it's just the case that we don't really
need to do anything at all for UTRACE_RESUME, i.e. we should just never
call finish_resume_report() in that case.
You cited the one most obvious case: utrace_get_signal() has just run, so
finish_resume_report() has just run and everything is already as we want.
What else?
* TIF_NOTIFY_RESUME spuriously set for some unrelated reason
(arch purposes)
- want to do nothing
* utrace_signal_handler set it
- want to do nothing more after clearing utrace-signal_handler
* TIF_NOTIFY_RESUME just set by utrace_control()
- it also set utrace-resume != UTRACE_RESUME, so not this case
It feels like there should be more corners, but they are not coming to mind
immediately. For just those cases, I think the change below would do it.
What do you think?
Thanks,
Roland
diff --git a/kernel/utrace.c b/kernel/utrace.c
index 34c990a..000 100644
--- a/kernel/utrace.c
+++ b/kernel/utrace.c
@@ -1866,8 +1866,18 @@ void utrace_resume(struct task_struct *t
*/
report.action = start_report(utrace);
- if (report.action == UTRACE_REPORT
- likely(task-utrace_flags UTRACE_EVENT(QUIESCE))) {
+ switch (report.action) {
+ case UTRACE_RESUME:
+ /*
+* Anything we might have done was already handled by
+* utrace_get_signal(), or this is an entirely spurious
+* call. (The arch might use TIF_NOTIFY_RESUME for other
+* purposes as well as calling us.)
+*/
+ return;
+ case UTRACE_REPORT:
+ if (unlikely(!(task-utrace_flags UTRACE_EVENT(QUIESCE
+ break;
/*
* Do a simple reporting pass, with no specific
* callback after report_quiesce.
@@ -1875,13 +1885,15 @@ void utrace_resume(struct task_struct *t
report.action = UTRACE_RESUME;
list_for_each_entry(engine, utrace-attached, entry)
start_callback(utrace, report, engine, task, 0);
- } else {
+ break;
+ default:
/*
* Even if this report was truly spurious, there is no need
* for utrace_reset() now. TIF_NOTIFY_RESUME was already
* cleared--it doesn't stay spuriously set.
*/
report.spurious = false;
+ break;
}
/*
Re: [PATCH] utrace: utrace_attach_task() forgets to return when -utrace == NULL
Just in case fyi, I cooked the almost identical patch yesterday, but
it didn't help: make xcheck crashes. Not that I really expected it can
help on x86.
Right.
Not sure I understand exactly... Yes, sometimes we know we don't need
read_barrier_depends(). For example,
@@ -302,7 +309,7 @@ struct utrace_engine *utrace_attach_task(
if (!utrace) {
if (unlikely(!utrace_task_alloc(target)))
return ERR_PTR(-ENOMEM);
- utrace = target-utrace;
+ utrace = task_utrace_struct(target);
}
I think this change is not necessary (but imho good anyway), this path
must take task_lock() and thus it must always see the correct *utrace.
Ok. If we were to change it we should add a comment about that.
But read_barrier_depends() does nothig except on alpha, why should we
care?
I thought this was the barrier you were talking about. Anyway, we should
be pedantically correct about these. (People do even still build Linux/Alpha.)
This reminds me, utrace_interrupt_pending() and similar code needs rmb()
or task-utrace != NULL check.
I take it you mean that:
if (task-utrace_flags)
... use task-utrace ...
paths need a barrier to ensure task-utrace_flags read nonzero implies
task-utrace will be read non-null when racing with the first attach.
Right?
This only needs smp_rmb(), right? Is there an existing explicit barrier
this pairs with? The corresponding smp_wmb() needs to be somewhere between
utrace_task_alloc()'s task-utrace = utrace; and utrace_add_engine():
if (!target-utrace_flags) {
target-utrace_flags = UTRACE_EVENT(REAP);
Right? A lot goes on in between there that probably has some implied
barriers (task_unlock, kmem_cache_alloc, spin_lock). But do we really
have the exact barrier we need? Or maybe we need:
/*
* This barrier makes sure the initialization of the struct
* precedes the installation of the pointer. This pairs
* with read_barrier_depends() in task_utrace_struct().
*/
wmb();
task-utrace = utrace;
/*
* This barrier pairs with task_utrace_struct()'s smp_rmb().
* It ensures this task-utrace store is always seen before
* task-utrace_flags is first set in utrace_add_engine().
*/
smp_wmb();
This is basically all the tracehook.h paths calling into utrace.
I think the easiest thing to do is to assume that use pattern and
also roll this barrier into task_utrace_struct().
diff --git a/include/linux/utrace.h b/include/linux/utrace.h
index 8924783..000 100644
--- a/include/linux/utrace.h
+++ b/include/linux/utrace.h
@@ -157,7 +157,18 @@ static inline unsigned long task_utrace_
static inline struct utrace *task_utrace_struct(struct task_struct *task)
{
- struct utrace *utrace = task-utrace;
+ struct utrace *utrace;
+
+ /*
+* This barrier ensures that any prior load of task-utrace_flags
+* is ordered before this load of task-utrace. We use those
+* utrace_flags checks in the hot path to decide to call into
+* the utrace code. The first attach installs task-utrace before
+* setting task-utrace_flags nonzero, with a barrier between.
+*/
+ smp_rmb();
+ utrace = task-utrace;
+
read_barrier_depends(); /* See utrace_task_alloc(). */
return utrace;
}
Is that what you have in mind?
Thanks,
Roland
Re: [PATCH 133] stepping, accommodate to utrace-cleanup changes
On 11/16, Oleg Nesterov wrote:
And I didn't check make xcheck, I guess it still crashes the kernel.
Yes it does. I am almost sure the bug should be trivial, but
somehow can't find find it. Just fyi, to ensure this is connected
to utrace-indirect I applied the hack below and the bug goes away.
OK. Tomorrow I will just read utrace.c trying to understand the
details of the new code, perhaps I will notice something.
Oleg.
--- UTRACE-PTRACE/kernel/fork.c~XXX_CRASH 2009-11-16 20:26:23.0
+0100
+++ UTRACE-PTRACE/kernel/fork.c 2009-11-16 21:18:57.0 +0100
@@ -970,6 +970,8 @@ static void posix_cpu_timers_init(struct
* parts of the process environment (as per the clone
* flags). The actual kick-off is left to the caller.
*/
+bool utrace_task_alloc(struct task_struct *task);
+
static struct task_struct *copy_process(unsigned long clone_flags,
unsigned long stack_start,
struct pt_regs *regs,
@@ -1227,6 +1229,10 @@ static struct task_struct *copy_process(
cgroup_fork_callbacks(p);
cgroup_callbacks_done = 1;
+ p-utrace = NULL;
+ if (!utrace_task_alloc(p))
+ WARN_ON(1);
+
/* Need tasklist lock for parent etc handling! */
write_lock_irq(tasklist_lock);
--- UTRACE-PTRACE/kernel/utrace.c~XXX_CRASH 2009-11-16 20:31:38.0
+0100
+++ UTRACE-PTRACE/kernel/utrace.c 2009-11-16 21:19:26.0 +0100
@@ -87,9 +87,9 @@ module_init(utrace_init);
*
* This returns false only in case of a memory allocation failure.
*/
-static bool utrace_task_alloc(struct task_struct *task)
+bool utrace_task_alloc(struct task_struct *task)
{
- struct utrace *utrace = kmem_cache_zalloc(utrace_cachep, GFP_KERNEL);
+ struct utrace *utrace = kzalloc(sizeof(struct utrace), GFP_KERNEL);
if (unlikely(!utrace))
return false;
spin_lock_init(utrace-lock);
@@ -108,7 +108,7 @@ static bool utrace_task_alloc(struct tas
}
task_unlock(task);
if (unlikely(task-utrace != utrace))
- kmem_cache_free(utrace_cachep, utrace);
+ kfree(utrace);
return true;
}
@@ -118,7 +118,7 @@ static bool utrace_task_alloc(struct tas
*/
void utrace_free_task(struct task_struct *task)
{
- kmem_cache_free(utrace_cachep, task-utrace);
+ kfree(task-utrace);
}
/*
@@ -286,6 +286,8 @@ struct utrace_engine *utrace_attach_task
struct utrace_engine *engine;
int ret;
+ WARN_ON(!utrace);
+
if (!(flags UTRACE_ATTACH_CREATE)) {
if (unlikely(!utrace))
return ERR_PTR(-ENOENT);
--- UTRACE-PTRACE/include/linux/utrace.h~XXX_CRASH 2009-11-16
01:06:34.0 +0100
+++ UTRACE-PTRACE/include/linux/utrace.h2009-11-16 20:50:38.0
+0100
@@ -163,7 +163,7 @@ static inline struct utrace *task_utrace
static inline void utrace_init_task(struct task_struct *task)
{
task-utrace_flags = 0;
- task-utrace = NULL;
+ WARN_ON(!task-utrace);
}
void utrace_release_task(struct task_struct *);
Re: [PATCH 133] stepping, accommodate to utrace-cleanup changes
Whatever temporary hacks are fine by me one way or the other. They will just be coming out later along with assorted other cleanup. We certainly do want to get this right in the utrace layer. The change we talked about before seems simple enough and should cover this without new kludges in the ptrace layer. I did this (commit f19442c). I think that should cover what you need without this ptrace change. (All untested, of course!) Thanks, Roland
Re: [PATCH 133] stepping, accommodate to utrace-cleanup changes
Yes it does. I am almost sure the bug should be trivial, but somehow can't find find it. Just fyi, to ensure this is connected to utrace-indirect I applied the hack below and the bug goes away. Does s/kmem_cache_zalloc/kzalloc/ really have anything to do with it? Isn't it just the allocation synchronization logic? OK. Tomorrow I will just read utrace.c trying to understand the details of the new code, perhaps I will notice something. Ok. I hope you'll find something not too strange. But if it takes long, then probably we should just change utrace_init_task() to eagerly call utrace_task_alloc() and let you work through other issues first. Thanks, Roland
Re: [PATCH] utrace: utrace_attach_task() forgets to return when -utrace == NULL
On 11/16, Roland McGrath wrote:
But read_barrier_depends() does nothig except on alpha, why should we
care?
I thought this was the barrier you were talking about. Anyway, we should
be pedantically correct about these. (People do even still build
Linux/Alpha.)
Yes, sure. I meant, we shouldn't worry that this barrier adds too much
overhead to task_utrace_struct().
This reminds me, utrace_interrupt_pending() and similar code needs rmb()
or task-utrace != NULL check.
I take it you mean that:
if (task-utrace_flags)
... use task-utrace ...
paths need a barrier to ensure task-utrace_flags read nonzero implies
task-utrace will be read non-null when racing with the first attach.
Right?
Yes,
This only needs smp_rmb(), right?
I think yes.
Is there an existing explicit barrier
this pairs with? The corresponding smp_wmb() needs to be somewhere between
utrace_task_alloc()'s task-utrace = utrace; and utrace_add_engine():
if (!target-utrace_flags) {
target-utrace_flags = UTRACE_EVENT(REAP);
We don't have the explicit barrier, but I think it is not needed.
In this case utrace_attach_task() does unlock+lock at least once
before changing -utrace_flags, this implies mb().
static inline struct utrace *task_utrace_struct(struct task_struct *task)
{
- struct utrace *utrace = task-utrace;
+ struct utrace *utrace;
+
+ /*
+ * This barrier ensures that any prior load of task-utrace_flags
+ * is ordered before this load of task-utrace. We use those
+ * utrace_flags checks in the hot path to decide to call into
+ * the utrace code. The first attach installs task-utrace before
+ * setting task-utrace_flags nonzero, with a barrier between.
+ */
+ smp_rmb();
+ utrace = task-utrace;
+
read_barrier_depends(); /* See utrace_task_alloc(). */
return utrace;
}
Yes, I think this patch should close this (pure theoretical) problem.
But this smp_rmb() in task_utrace_struct() is only needed when the
caller does something like
if (task_utrace_flags(tsk))
do_something(task_utrace_struct());
it would be more logical to move this rmb() info task_utrace_flags().
However task_utrace_flags() must be fast.
Perhaps, to avoid this smp_rmb(), we can change
bool utrace_interrupt_pending(void)
{
struct utrace *utrace = task_utrace_struct(current);
return utrace utrace-resume == UTRACE_INTERRUPT;
}
I dunno.
Oleg.
Re: [PATCH] utrace: utrace_attach_task() forgets to return when -utrace == NULL
Yes, sure. I meant, we shouldn't worry that this barrier adds too much
overhead to task_utrace_struct().
Ah! Sorry, I misread you. Yes, good point.
We don't have the explicit barrier, but I think it is not needed.
In this case utrace_attach_task() does unlock+lock at least once
before changing -utrace_flags, this implies mb().
Ok, this is exactly the sort of thing that merits explicit comments.
Yes, I think this patch should close this (pure theoretical) problem.
But this smp_rmb() in task_utrace_struct() is only needed when the
caller does something like
if (task_utrace_flags(tsk))
do_something(task_utrace_struct());
If you look at where task_utrace_struct() is used, it's basically always
like this. All the tracehook.h callers into utrace.c do that.
it would be more logical to move this rmb() info task_utrace_flags().
However task_utrace_flags() must be fast.
Right. The hot path is that you test task-utrace_flags and it's zero.
It seems better not to add anything else to that path, not even lfence.
Perhaps, to avoid this smp_rmb(), we can change
bool utrace_interrupt_pending(void)
{
struct utrace *utrace = task_utrace_struct(current);
return utrace utrace-resume == UTRACE_INTERRUPT;
}
I dunno.
But that's not the only place, is it? Every utrace_report_* and most every
other utrace.c entry point is called from tracehook.h code using this pattern.
Those can have the same issue. So you'd have to make all of those do an:
if (unlikely(!utrace))
return;
sort of check. Is that what you mean?
I guess we'd need some chip-knowledgeable microoptimizers to tell us
whether:
smp_rmb(); /* guarantees we can't be here seeing utrace==NULL */
utrace = task-utrace;
is better or worse than:
utrace = task-utrace;
if (unlikely(!utrace))
return;
on today's chips.
The latter is more hairy for the utrace.c code to always be sure to check.
But it's not prohibitively so.
Thanks,
Roland
