Q: utrace_prepare_examine() && get_task_struct(target)

[Roland McGrath]

> >  		engine->ops = NULL;
> > +		engine->flags = 0;
> >  		list_move(&engine->entry, &detached);
> 
> I think this makes sense regardless.

Agreed.

> > +	 * Make sure all engine->flags and engine->ops updates are done
> > +	 * before the current task_struct might really die.
> > +	 */
> > +	smp_wmb();
> 
> I don't think we need additional barriers here or in prepare/finish,
> rcu_call() should play correctly with rcu_read_lock().
> 
> IOW, rcu_read_lock() must have enough "barrierness" wrt rcu_call().

Right, OK.

> > The other calls use get_utrace_lock to implement this guarantee.  It checks
> > that your engine is attached before it looks at the task_struct, and this
> > is (supposedly) synchronized with detach and reap safely.  (You should be
> > checking on that supposition, Oleg. ;-)
> 
> I still think EXIT_DEAD check must die ;)

In get_utrace_lock, you mean?  Do you mean it's superfluous because we can
rely on utrace_reap having cleared engine->ops before it matters to us?

> Hmm. I think without CONFIG_PREEMPT_RCU schedule_debug() will complain
> if wait_task_inactive() blocks.

Ok, I thought something like that might be true.

> In any case, without CONFIG_PREEMPT_RCU we must not schedule() under
> rcu_read_lock(), because rcuclassic considers schedule() call as
> "->passed_quiesc = true".

That's exactly what I meant by "lets RCU stuff happen".  But I was
supposing it would work because after you come out of schedule() it's as if
you'd done rcu_read_unlock();rcu_read_lock(); before put_task_struct().

But anyway, I was just illustrating what might be the very best you can do
with what we have now, to say that it is clearly inadequate.

> > This suggests perhaps utrace_prepare_examine ought to keep a task ref to
> > be released by utrace_finish_examine.  But then it's a back-door way for
> > modules to use get/put_task_struct and leak task_structs.  Is that OK?
> 
> Oh, don't ask me ;)

Well, I was hoping we might get some other people into the discussion too.

> But I think this is very natural. Yes, with this change the caller must
> always do utrace_finish_examine() if _prepare_ succeeds, but a buggy
> utrace module can leak memory or just crash the kernel in many ways.

Sure.  Of course it can do get_task_struct() today, and just is guaranteed
to leak since it can't do put_task_struct().

> Or we can export __put_task_struct(), this means more work for utrace
> user.

Right, we should be finding the utrace API that makes it easiest for users
to get things right, that's primary.  I was just concerned that upstream
folks might say that modules can't do put_task_struct() and that's how they
want it, and that being so they don't want a backdoor method to do
get_task_struct()/put_task_struct() either because whatever bad modules
they are worried about would exploit it.

> Or. Actually, when I started read utrace.c I was surprised there is
> no "task_struct *tracee" in "struct utrace_engine". Perhaps engine
> itself should ping task_struct ? And engine->tracee can be very handy.

I don't think a detached struct utrace_engine should pin any task_struct.
That just makes utrace_engine_{get,put} a proxy for {get,put}_task_struct.
And if it doesn't, then it's not much different from what we have now, is
it?  That is, given unpredictable asynchronous detach because of SIGKILL or
mt-exec + parent wait or self-reaping.  Perhaps I'm wrong to want to avoid
keeping task_struct refs like that.  I'm open to opinions.

There are other API reasons that I never wanted a task_struct pointer in
struct utrace_engine.  But we can get into those later.  It's moot if we
concur with my idea that holding task_struct refs past detach is bad.


Thanks,
Roland