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 <o...@redhat.com>
---
--- __UTRACE/include/linux/utrace.h~4_DTOR 2009-08-14 11:58:05.000000000
+0200
+++ __UTRACE/include/linux/utrace.h 2009-08-14 16:44:27.000000000 +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.000000000 +0200
+++ __UTRACE/kernel/utrace.c 2009-08-14 16:29:37.000000000 +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);
