Re: [RFC, PATCH] teach utrace to destroy engine->data

2009-09-04 Thread Oleg Nesterov
On 09/04, Ananth N Mavinakayanahalli wrote:
>
> I prefer Roland's ops->release approach.

Yes! me too.

> However, is there any chance
> that ->ops is no longer valid

Sure, ->ops can be changed at "any time". For example, just because of
UTRACE_DETACH.

That is why utrace_attach_task() saves ->release pointer in engine.
But this is transparent for the user, and engine->release is private.
We can change the implementation so that ->ops is never changed.

> The concept of how utrace engines use refcnts initially befuddled me.
> The entity that does a utrace_attach_*, without it having to explicitly
> ask for a utrace_engine_get(), already owns a reference. That was
> counter intuitive.

Otherwise, how can we use the returned engine? The task can be killed
right after utrace_attach_ and disappear. In this case release_task()
path must do utrace_engine_put().

This looks very natural to me.

Oleg.



Re: [RFC, PATCH] teach utrace to destroy engine->data

2009-09-04 Thread Ananth N Mavinakayanahalli
On Fri, Sep 04, 2009 at 02:55:29AM -0700, Roland McGrath wrote:
> > Simple answer. Because I do not know how to implement this. At least now.
> > I tried to think of this more, but I don't see how to make the first steps.
> > 
> > (Yes, to be honest, this looks like "unnecessary complication" to me, I have
> >  to admit. But this is not the reason.)
> 
> Well, I am befuddled.  I explained the model and it seems obvious to me how
> you'd go about it, the only nontrivial corner being coping with the MT exec
> reap case.  But I won't let our mutual befuddlement stand in the way of
> making immediate progress the way you like best.
> 
> > Agreed, but nobody else cares ;)
> 
> Ananth, Srikar, Jim, Frank?  (Beuller?)  We do have some people around
> here with some experience dealing with the API.  They made me add the
> engine ref counts in the first place, for Pete's sake.  They've got to
> have some opinions!

I am not sure if all of what I say here makes any sense as I am still in
the midst of understanding the discussion threads between the two of you
(admittedly, its one hairy piece of the kernel). However, here goes:

I prefer Roland's ops->release approach. However, is there any chance
that ->ops is no longer valid at the time __utrace_release_engine() is
called? For the ptrace case at least, it'll always be valid, so there
are no issues.

The concept of how utrace engines use refcnts initially befuddled me.
The entity that does a utrace_attach_*, without it having to explicitly
ask for a utrace_engine_get(), already owns a reference. That was
counter intuitive. (That's the reason for a series of Srikar's
utrace_engine_put() fixes too, I guess).

That apart, the problem of freeing ->data is similar to what Prasad is
trying to grapple in hardware watchpoints and its ptrace interaction. I
guess its a problem unique to ptrace(?). The issue there, is that when
ptrace did a register_user_hardware_watchpoint(), and that thread later
died/exec'd, ptrace may (will?) not actually unregister the watchpoint,
and the hwbkpt layer is left to fend for itself in cleaning up the
struct hw_breakpoint in flush_thread_hardware_watchpoint(). Now, you
could have non-ptrace users of the interface (who may have passed just
a reference to a statically allocated struct hw_breakpoint) in which case,
freeing the structure in the hwbkpt layer is just wrong. Right now, the
way Prasad has fixed it is to have a check if the
bp->triggered == ptrace_triggered, in which case, free up the structure.

Yes, there is a kind of layering violation here -- hwbkpt now knows
about ptrace_triggered. But I am not sure if there is a better way of
fixing it.

Ananth



Re: [RFC, PATCH] teach utrace to destroy engine->data

2009-09-04 Thread Roland McGrath
> Simple answer. Because I do not know how to implement this. At least now.
> I tried to think of this more, but I don't see how to make the first steps.
> 
> (Yes, to be honest, this looks like "unnecessary complication" to me, I have
>  to admit. But this is not the reason.)

Well, I am befuddled.  I explained the model and it seems obvious to me how
you'd go about it, the only nontrivial corner being coping with the MT exec
reap case.  But I won't let our mutual befuddlement stand in the way of
making immediate progress the way you like best.

> Agreed, but nobody else cares ;)

Ananth, Srikar, Jim, Frank?  (Beuller?)  We do have some people around
here with some experience dealing with the API.  They made me add the
engine ref counts in the first place, for Pete's sake.  They've got to
have some opinions!

> So, I am going to use the simple ops->release method for now. Once we
> have the working code we can reconsider the lifetime rules for engine/
> engine->data.

Agreed.


Thanks,
Roland



Re: [RFC, PATCH] teach utrace to destroy engine->data

2009-09-02 Thread Oleg Nesterov
On 08/23, Roland McGrath wrote:
>
> > > This model seems like a better fit for ptrace to me.  The anchoring data
> > > structure is the tracer's tracees list, which links together the
> > > ptrace_context structs.  A live ptrace_context has an engine pointer and
> > > a ref for it.
> >
> > OK. Not that I really understand this all in details, but OK.
> >
> > But. Do you think we should do this right now?
>
> If it is the right way to handle the data structure lifetimes, I don't see
> why we would do it any different way first.  One of the main points of what
> makes one plan or another "the right way" is that it makes it easier to get
> the corners right.  So if it's easier to get the corners right another way,
> then maybe that other way is the right one.
>
> > I don't. Imho, we need a lot of changes before this.
>
> I don't understand why you think this.

Simple answer. Because I do not know how to implement this. At least now.
I tried to think of this more, but I don't see how to make the first steps.

(Yes, to be honest, this looks like "unnecessary complication" to me, I have
 to admit. But this is not the reason.)

> > Do you have any suggestion what can we do right now? (assuming you won't
> > apply your ops->release patch).
>
> I didn't say I wouldn't.  I was hoping for some more discussion about it
> and better understanding of the underlying issues that made you want it,
> maybe with some voices other than just yours and mine.

Agreed, but nobody else cares ;)


So, I am going to use the simple ops->release method for now. Once we
have the working code we can reconsider the lifetime rules for engine/
engine->data.

Oleg.



Re: [RFC, PATCH] teach utrace to destroy engine->data

2009-08-23 Thread Roland McGrath
> > This model seems like a better fit for ptrace to me.  The anchoring data
> > structure is the tracer's tracees list, which links together the
> > ptrace_context structs.  A live ptrace_context has an engine pointer and
> > a ref for it.
> 
> OK. Not that I really understand this all in details, but OK.
> 
> But. Do you think we should do this right now?

If it is the right way to handle the data structure lifetimes, I don't see
why we would do it any different way first.  One of the main points of what
makes one plan or another "the right way" is that it makes it easier to get
the corners right.  So if it's easier to get the corners right another way,
then maybe that other way is the right one.

> I don't. Imho, we need a lot of changes before this. 

I don't understand why you think this.

> We need the working code first. I just can't understand how can we do the
> necessary changes to solve (say) the problems with de_thread() now, when
> it is not clear (to me) how the basic/core ptrace code will look.

This subject is part of how we organize the basic/core ptrace data
structures.  I don't think we have to make things like the MT exec case
perfect right away.  In fact, I think that wrinkle is the only thing that
makes the "hold another ref" model I described not completely simple and
straightforward to apply.

> Do you have any suggestion what can we do right now? (assuming you won't
> apply your ops->release patch).

I didn't say I wouldn't.  I was hoping for some more discussion about it
and better understanding of the underlying issues that made you want it,
maybe with some voices other than just yours and mine.

I'm having trouble understanding why you think I've suggested some new
tangent of work bigger or different from what you're already engaged in.
So I'm at a bit of a loss for what different thing you're hoping I can
suggest.

> > I don't really follow this at all.
> > Your ptrace code has nothing to do with utrace->lock, and never will.
> 
> Yes, agreed. ptrace should not use utrace->lock. But currently I don't
> see how to do this without utrace->lock. Or without rcu + refcnt.

Sorry, but I still really have no idea what you are thinking about here.
If you are considering overloading this unrelated lock for some purpose,
you'll have to explain how you want to use it and why that's necessary.


Thanks,
Roland



Re: [RFC, PATCH] teach utrace to destroy engine->data

2009-08-19 Thread Oleg Nesterov
On 08/18, Roland McGrath wrote:
>
> > > (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...
>
> UTRACE_EVENT(QUIESCE) constitutes "hooking death".

Ah yes, utrace_report_death() doesn't call ->report_death() directly,
it does REPORT_CALLBACKS().

> > I must have missed something.
> >
> > 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.
>
> What you missed is where I said this model is appropriate for situations
> where you don't need to do that:

Yes, I did miss that.

> > > The second model is where you normally hold a utrace_engine reference
> [...]
> > And this means we need some hairy tricks.
>
> That is an utterly vague blanket statement to make.

Agreed. Let me try again.

> This model seems like a better fit for ptrace to me.  The anchoring data
> structure is the tracer's tracees list, which links together the
> ptrace_context structs.  A live ptrace_context has an engine pointer and
> a ref for it.

OK. Not that I really understand this all in details, but OK.

But. Do you think we should do this right now?

I don't. Imho, we need a lot of changes before this. We need the working
code first. I just can't understand how can we do the necessary changes
to solve (say) the problems with de_thread() now, when it is not clear
(to me) how the basic/core ptrace code will look.

And to have the more or less working code, we need engine->data which
can be used by tracer.

Do you have any suggestion what can we do right now? (assuming you won't
apply your ops->release patch).

> > 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...
>
> I don't really follow this at all.
> Your ptrace code has nothing to do with utrace->lock, and never will.

Yes, agreed. ptrace should not use utrace->lock. But currently I don't
see how to do this without utrace->lock. Or without rcu + refcnt.

Oleg.



Re: [RFC, PATCH] teach utrace to destroy engine->data

2009-08-18 Thread Roland McGrath
> > (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...

UTRACE_EVENT(QUIESCE) constitutes "hooking death".
A report_quiesce (UTRACE_EVENT(DEATH)) call precedes the report_death call.

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

That really has nothing to do with utrace.  That's another "you" path that
you control and can make do what you need it to.  But, it is immaterial to
the point I made: you can use report_reap as the means to interlock with
that too if you so chose.  It seems like a goofy choice for that, since it
would be your code->release_task()->utrace_release_task()->your report_reap()
and you might as well just do your code->your code.

> > 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.
[...]
> 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.

What you missed is where I said this model is appropriate for situations
where you don't need to do that:

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

> > The second model is where you normally hold a utrace_engine reference
[...]
> And this means we need some hairy tricks.

That is an utterly vague blanket statement to make.

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

I don't really follow this at all.  
Your ptrace code has nothing to do with utrace->lock, and never will.


Thanks,
Roland



Re: [RFC, PATCH] teach utrace to destroy engine->data

2009-08-18 Thread Oleg Nesterov
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 

Re: [RFC, PATCH] teach utrace to destroy engine->data

2009-08-18 Thread Oleg Nesterov
On 08/17, Roland McGrath wrote:
>
> If we do add the destructor hook, I think it looks like this.
> ...
> @@ -271,6 +273,7 @@ struct utrace_engine *utrace_attach_task(
>   engine->flags = 0;
>   engine->ops = ops;
>   engine->data = data;
> + engine->release = ops->release;

Agreed! this is much better.

Oleg.



Re: [RFC, PATCH] teach utrace to destroy engine->data

2009-08-17 Thread Roland McGrath
If we do add the destructor hook, I think it looks like this.
It would merit some write-up in utrace.tmpl text too.

I'm not especially convinced it's a very good idea.
But I won't object if people serious about using the API think it helps.


Thanks,
Roland


---
 include/linux/utrace.h |8 
 kernel/utrace.c|3 +++
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/include/linux/utrace.h b/include/linux/utrace.h
index c19c01b..f968792 100644
--- a/include/linux/utrace.h
+++ b/include/linux/utrace.h
@@ -322,6 +322,7 @@ static inline enum utrace_syscall_action 
utrace_syscall_action(u32 action)
 struct utrace_engine {
 /* private: */
struct kref kref;
+   void (*release)(void *);
struct list_head entry;
 
 /* public: */
@@ -539,6 +540,12 @@ static inline void utrace_engine_put(struct utrace_engine 
*engine)
  * Unlike other callbacks, this can be called from the parent's context
  * rather than from the traced thread itself--it must not delay the
  * parent by blocking.
+ *
+ * @release:
+ * If not %NULL, this is called after the last utrace_engine_put()
+ * call for a &struct utrace_engine, which could be implicit after
+ * a %UTRACE_DETACH return from another callback.  Its argument is
+ * the engine's @data member.
  */
 struct utrace_engine_ops {
u32 (*report_quiesce)(enum utrace_resume_action action,
@@ -584,6 +591,7 @@ struct utrace_engine_ops {
bool group_dead, int signal);
void (*report_reap)(struct utrace_engine *engine,
struct task_struct *task);
+   void (*release)(void *data);
 };
 
 /**
diff --git a/kernel/utrace.c b/kernel/utrace.c
index 35be909..3b7ac74 100644
--- a/kernel/utrace.c
+++ b/kernel/utrace.c
@@ -79,6 +79,8 @@ void __utrace_engine_release(struct kref *kref)
struct utrace_engine *engine = container_of(kref, struct utrace_engine,
kref);
BUG_ON(!list_empty(&engine->entry));
+   if (engine->release)
+   (*engine->release)(engine->data);
kmem_cache_free(utrace_engine_cachep, engine);
 }
 EXPORT_SYMBOL_GPL(__utrace_engine_release);
@@ -271,6 +273,7 @@ struct utrace_engine *utrace_attach_task(
engine->flags = 0;
engine->ops = ops;
engine->data = data;
+   engine->release = ops->release;
 
ret = utrace_attach_delay(target);
if (likely(!ret))



Re: [RFC, PATCH] teach utrace to destroy engine->data

2009-08-17 Thread Roland McGrath
> 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 (or even just report_quiesce
when passed UTRACE_EVENT(DEATH)) that always detaches.  In that event you
don't really need report_reap.  (In theory, this is how ptrace would be.
It doesn't have any use for utrace once a task has died.  But in practice
it depends entirely on how the wait interference is done--that is outside
of utrace proper, but you could conceivably want to use report_reap as the
mechanism for synchronizing with de_thread(), which is the one place that
ordinarily preempts the ptrace wait interference.)

> Note that this itself adds some complications. But the real problem
> is: the tracer can never safely dereference engine->data, the tracee
> can be killed/released at any moment.

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;

The logic is the same for "disable" rather than "detach", using
utrace_set_events(task, engine, 0) rather than utrace_control().  
The same return values tell you if there is a death race.  In either
case, you'd follow up with utrace_barrier() for -EINPROGRESS unless
the only kind of race that's of concern for your case is a death race
(such as when you're sure it was stopped in a place where you could
get no other callbacks, or you don't have other callbacks anyway).

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.
You might have whatever callbacks that decide to use UTRACE_DETACH if
that's appropriate.  But these would not necessarily free your data
structures.  Instead, the control path does that well after detach,
when it does the final utrace_engine_put().

This model seems like a better fit for ptrace to me.  The anchoring data
structure is the tracer's tracees list, which links together the
ptrace_context structs.  A live ptrace_context has an engine pointer and
a ref for it.  When a tracee dies, it uses UTRACE_DETACH and so drops
the implicit engine ref, leaving only the ptrace_context's ref.  The
tracer cleans this all up when it fake-reaps the zombie in wait.  

There is whatever non-utrace magic to turn off the wait interference,
and also the de_thread() race vs wait interference.  But those really
are special to ptrace.  For de_thread() you could do something hairy
with report_reap, or you could just handle it lazily with a task ref and
EXIT_DEAD checks to clean up and ignore it in the next wait.  (So all
the MT exec zombies will be released but not __put_task_struct until the
debugger waits after the PTRACE_EVENT_EXEC/SIGTRAP stop.  Or you could
even make the debugger see all the "zombies" that MT-exec now robs it
of, since their ->exit_code is all it was really going to get anyway and
you can still see it just as well in EXIT_DEAD.  I guess that's not
really any good as a feature if the pid could be reused.)

In the general case, I would expect asynchronous controllers to be going
from their own data structures to utrace_engine + task_struct/struct pid
rather than the other way around.  But you can still use a utrace_attach
lookup if you like.  (It seems handy because it's behind an API, but
you're really just choosing O(n) in the number of unrelated engines on
that task vs O(n) in the number of tracees walking your own data
structure.  That's probably the right choice for ptrace, but you always
have lots of choices.)  J

[RFC, PATCH] teach utrace to destroy engine->data

2009-08-14 Thread Oleg Nesterov
On 07/23, Oleg Nesterov wrote:
>
> On 07/22, Roland McGrath wrote:
> >
> > > Who will kfree() engine->data ?
> > >
> > > sys_ptrace(PTRACE_DETACH) can do this. But what if the tracer exits
> > > and detaches? Or release_task() does untrace.
> >
> > All of these are some ptrace.c path that has to use UTRACE_DETACH,
> ...
>
> Hmm. Yes, looks like you are right... Not sure what I was worried
> about. I did have some concerns, but I guess I was wrong. Good!

Now I recall why I was worried. And in fact, I think that without
some change utrace->engine is hardly useable.

Let's consider ptrace, but of course it is not special. We introduce
struct ptrace_context {...} and change ptrace_attach() to setup
engine->data to be a pointer to ptrace_context.

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.

Note that this itself adds some complications. But the real problem
is: the tracer can never safely dereference engine->data, the tracee
can be killed/released at any moment.

Sure. We can protect ->data by RCU, or add refcounting, etc. But
this is nightmire. This is complex and (I bet) will lead to numerous
bugs.

I strongly believe that ->data should be freed by utrace_engine_put().
If you have the reference to engine, engine->data must be stable.


So, this patch adds engine->dtor and utrace_engine_set_dtor() helper.
It would be better to pass dtor as yet another argument to
utrace_attach_task(). But this means more changes, and in this case
it is better to have utrace_attach_create() and utrace_attach_lookup(),
the new argument is only needed with UTRACE_ATTACH_CREATE.

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.

Thoughts?

Signed-off-by: Oleg Nesterov 
---


--- __UTRACE/include/linux/utrace.h~4_DTOR  2009-08-14 11:58:05.0 
+0200
+++ __UTRACE/include/linux/utrace.h 2009-08-14 16:44:27.0 +0200
@@ -322,6 +322,7 @@ static inline enum utrace_syscall_action
 struct utrace_engine {
 /* private: */
struct kref kref;
+   void (*dtor)(void *data);
struct list_head entry;
 
 /* public: */
@@ -342,6 +343,15 @@ static inline void utrace_engine_get(str
kref_get(&engine->kref);
 }
 
+/**
+ * utrace_engine_set_dtor - setup the destructor for ->data
+ */
+static inline void utrace_engine_set_dtor(struct utrace_engine *engine,
+   void (*dtor)(void *))
+{
+   engine->dtor = dtor;
+}
+
 void __utrace_engine_release(struct kref *);
 
 /**
@@ -349,7 +359,8 @@ void __utrace_engine_release(struct kref
  * @engine:&struct utrace_engine pointer
  *
  * You must hold a reference on @engine, and you lose that reference.
- * If it was the last one, @engine becomes an invalid pointer.
+ * If it was the last one, @engine->dtor(@engine->data) is called and
+ * @engine becomes an invalid pointer.
  */
 static inline void utrace_engine_put(struct utrace_engine *engine)
 {
--- __UTRACE/kernel/utrace.c~4_DTOR 2009-08-14 13:54:54.0 +0200
+++ __UTRACE/kernel/utrace.c2009-08-14 16:29:37.0 +0200
@@ -79,6 +79,8 @@ void __utrace_engine_release(struct kref
struct utrace_engine *engine = container_of(kref, struct utrace_engine,
kref);
BUG_ON(!list_empty(&engine->entry));
+   if (engine->dtor)
+   engine->dtor(engine->data);
kmem_cache_free(utrace_engine_cachep, engine);
 }
 EXPORT_SYMBOL_GPL(__utrace_engine_release);