> > 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