> ptrace_attach_task:
>
> engine = utrace_attach_task(CREATE | EXCLUSIVE);
>
> err = utrace_set_events();
> WARN_ON(err && !tracee->exit_state);
>
> Looks correct but it is not. utrace_attach_task() can return EINPROGRESS.
utrace_set_events can, yes.
> Note that start_callback/etc sets ->reporting = engine even if we are not
> going to call ->report_any().
Yes, I believe this is necessary for the interlocks to work as described.
> This WARN_ON() was always wrong, even without multitracing, even without
> recent changes which introduced reusing.
Right.
> But I have to admit, I dislike this WARN_ON() in ptrace_attach_task().
> First, it needs the comment to explaint why EINPROGRESS is OK.
Right.
> But more importantly, WARN_ON(err && err != -EINPROGRESS &&
> !tracee->exit_state)
> equally applies to any usage of utrace_set_events(), no? If this warning
> is useful, then perhaps we should move it into utrace_set_events() ?
That condition amounts to asserting that utrace_set_events() meets its
specified interface, so it's not really useful, no. I what it checks is
that the ptrace-layer logic hadn't detached the engine from a live task.
That is a sanity check that might be useful, I guess that's why it's there.
As I mentioned in the message you cited, we could just make
utrace_set_events() a little less paranoid and have it only return
-EINPROGRESS when there is any meaningful race possible. That is,
when it's clearing any bits. So how about leaving ptrace's sanity
check alone and doing this instead?
diff --git a/kernel/utrace.c b/kernel/utrace.c
index 25737b5..a7bdd88 100644
--- a/kernel/utrace.c
+++ b/kernel/utrace.c
@@ -581,7 +581,8 @@ int utrace_set_events(struct task_struct *target,
set_tsk_thread_flag(target, TIF_SYSCALL_TRACE);
ret = 0;
- if (!utrace->stopped && target != current && !target->exit_state) {
+ if ((old_flags & ~events) != 0 &&
+ !utrace->stopped && target != current && !target->exit_state) {
/*
* This barrier ensures that our engine->flags changes
* have hit before we examine utrace->reporting,
Thanks,
Roland
