Re: [PATCH 23] ptrace_attach_task: kill WARN_ON(err) (perhars we can improve ->reporting logic?)

[Roland McGrath]

> 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.  I think the old code just predates
utrace_barrier() et al.  That can just be:

if (err && err != -EINPROGRESS)
WARN_ON(!tracee->exit_state);

to keep the sanity check.

> Note that start_callback/etc sets ->reporting = engine even if we are not
> going to call ->report_any(). I'll try to think, perhaps we can change this
> code to check engine->flags first...

I think the barrier logic in start_callback() won't allow that.

> Note that EINPROGRESS can happen even if there are no other tracers.

That's fine.  It just means "not synchronized yet".  I suppose we could
make utrace_set_events() skip it when it's not clearing any bits.

> But the real problem is, _sometimes_ "make xcheck" triggers this warning,
> and I can not understand:
> 
>   1. Why I didn't see it before context->options was added
> 
>   2. I changed utrace_resume() to check QUIESCE. But I still
>  see the warning!
> 
> Still investigating...

I don't have any answers to those off hand.


Thanks,
Roland

[PATCH 23] ptrace_attach_task: kill WARN_ON(err) (perhars we can improve ->reporting logic?)

[Oleg Nesterov]

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.

Note that start_callback/etc sets ->reporting = engine even if we are not
going to call ->report_any(). I'll try to think, perhaps we can change this
code to check engine->flags first...

Note that EINPROGRESS can happen even if there are no other tracers.
utrace_add_engine() sets ->report and the tracee can call utrace_resume()
before we do utrace_set_events().

I think that it makes sense to check utrace_flags & QUIESCE in utrace_resume()
and return if it is not set. But even with this change EINPROGRESS is still
possible if we have other engines.


But the real problem is, _sometimes_ "make xcheck" triggers this warning,
and I can not understand:

1. Why I didn't see it before context->options was added

2. I changed utrace_resume() to check QUIESCE. But I still
   see the warning!

Still investigating...

---

 kernel/ptrace.c |3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- PU/kernel/ptrace.c~23_ATTACH_KILL_WARNING   2009-09-04 19:50:35.0 
+0200
+++ PU/kernel/ptrace.c  2009-09-04 19:57:47.0 +0200
@@ -505,8 +505,7 @@ static int ptrace_attach_task(struct tas
 * It can fail only if the tracee is dead, the caller
 * must notice this before setting PT_PTRACED.
 */
-   err = __ptrace_set_options(tracee, engine, options);
-   WARN_ON(err && !tracee->exit_state);
+   __ptrace_set_options(tracee, engine, options);
utrace_engine_put(engine);
return 0;
 }