On 08/17, Roland McGrath wrote: > > > Now. we should make sure that every user of UTRACE_DETACH frees > > engine->data. And, we have to introduce ptrace_report_reap() which > > should free it too. > > Right, or you can have a report_death callback
Ah sorry, I meant ->report_death, not _reap. > (or even just report_quiesce > when passed UTRACE_EVENT(DEATH)) that always detaches. Hmm. Not sure I understand how can we do without hooking death or reap... > synchronizing with de_thread(), which is the one place that > ordinarily preempts the ptrace wait interference.) Not only de_thread(). If the tracee is killed, another thread can do wait()->release_task(). This means even _reap (instead of _death) can't save us. > I see two basic models for handling the engine lifetime issues in a utrace > engine. > > One way is where you do not normally hold a utrace_engine reference. > You get an initial one from UTRACE_ATTACH_CREATE when you do setup. > Once you have done utrace_set_events() and maybe utrace_control(), you > do utrace_engine_put() immediately. Then if you have callbacks that > use UTRACE_DETACH (or else have report_reap, where it's implicit) they > are responsible for engine->data. And indeed you must have some such > if you have an engine->data pointer that needs cleanup. This model is > probably a good fit for "passive" tracing, where the only thing you > really come back and do later is disable it (or for something simple > enough it doesn't need any cleanup for engine->data). Then you can do: > > engine = utrace_attach_task(task, UTRACE_ATTACH_MATCH_OPS, > &my_ops, NULL); > if (IS_ERR(engine)) ... > ret = utrace_control(task, engine, UTRACE_DETACH); > if (ret) ... > clean_up(engine->data); > > where -ESRCH/-EALREADY tell you that your report_reap/report_death > (also counts for report_quiesce in the report_death pass) either will > run, has run, or is running, so in that case you rely on them to do: > > clean_up(engine->data); > return UTRACE_DETACH; I must have missed something. Sure, it is more or less trivial to free engine->data safely. I mean, it is easy to avoid the double-free/etc problems. But I can't understand how the tracer can use ->data safely. Whatever the tracer does, the tracee can be killed at any moment. It can pass ->report_death/reap events under us. The tracer has 2 references, one to task_struct, another one to engine. That is why I still think a reference to engine should also pin engine->data, and I like very much the patch you sent. > The second model is where you normally hold a utrace_engine reference > to represent the life of your data structure, separate from the > implicit utrace_engine reference for being attached. The engine uses > the rule that the code that drops the last engine reference cleans up > the data structure, rather than the code that detaches doing that. And this means we need some hairy tricks. Perhaps I am overestimating this problem and these tricks are not that hairy. But still engine->release method is much simpler. > But I'm always open to adding things to make it easier > to write modules (and with fewer bugs), Yes! > > I strongly believe that ->data should be freed by utrace_engine_put(). > > If you have the reference to engine, engine->data must be stable. > > Well, that's a rule you can decide you want for your engine's handling > of its ->data, sure. You can in fact do that already, since you really > are in control of all the gets and puts. There is one put that's > implicit in detach, but you always either choose to detach or at least > could be notified. All other gets and puts are what your engine's code > chose to do. Yes sure. If you don't apply your patch which uses ops->release(), then I am going to add engine->data->in_use for reports which return UTRACE_DETACH or ->report_death. If the tracer needs engine->data it should use ptrace_get_context/ptrace_put_context. I am not sure we can avoid taking utrace->lock for that... > > So, this patch adds engine->dtor and utrace_engine_set_dtor() helper. > > The proper interface for this is just to add .release to utrace_engine_ops. > Because of how we fiddle the fields in detach, we'd still need to put the > function pointer into struct utrace_engine at some point in the > implementation. But the API doesn't really need to know about that. Yes, agreed. > > Afaics, unless a module intentionally leaks utrace_engine, it can safely > > use ->dtor() which lives in ->module_core. But at least it can always > > use ->dtor = kfree. > > I've never really figured out all the issues about module unloading for > utrace. I assume that's what you're referring to here. The two obvious > safe paths would seem to be either holding module refs whenever an > engine is attached (or now, until your release hook is called), or > synchronously detaching all engines (plus utrace_barrier et al) in the > module_exit function. Yes. Oleg.
