On 10/15, Roland McGrath wrote:
>
> > 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.
Yes. I'd like to _try_ to think if we can change this, but most
probably we can't.
> > 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,
Well, I think you are right... But I just can't convince myself I
_really_ understand why the "set new bits" case is not interesting.
Otoh, I can't imagine why the caller should worry about -EINPROGRESS
if it doesn't clear any events.
But I think the new code should have the comment to explain this
"old_flags & ~events" condition.
In any case I agree: if we change utrace_set_events() this way,
then we do not need to change WARN_ON() in attach.
Oleg.
