[PATCH 83] ptrace_attach_task: suppress WARN_ON(err)

[Oleg Nesterov]

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.