get_utrace_lock() must threat utrace->reap == T as engine->ops == NULL.
The logic in get_utrace_lock() is correct. If we see engine->ops != NULL
under rcu_read_lock(), the target has not passed release_task() yet, so
it is safe to dereference target/utrace and take utrace->lock.
However. If utrace->reap is true it is not safe to use target/utrace
after return from get_utrace_lock(), it drops rcu_read_lock() and returns
with utrace->lock held, but spin_lock() doesn't imply RCU lock.
IOW. Suppose that get_utrace_lock() is called after utrace_maybe_reap()
unlocks utrace->lock() and before it starts the last REAP reporting loop.
It is very possible engine has the valid ->ops != utrace_detached_ops,
in this case get_utrace_lock() succeeds. But, nothing protects from
release_task()->call_rcu(delayed_put_task_struct) after that, this means
that the caller of get_utrace_lock() can't access target/utrace after
rcu_read_unlock().
Let's see how this change affect the callers:
- utrace_barrier() - please ignore. It needs fixes with or
without this change.
- utrace_control() is not affected except this change makes
the utrace->reap check in utrace_control_dead() unnecessary.
- utrace_set_events() is obviously affected, but imho this
is good.
Really, we can do nothing with target/engine if ->reap is
true. What is the point to allow to change engine->flags
when we know that the target is under destruction?
Again, this makes utrace->reap check unnecessary, we can
remove it. I already sent the preparation changes which
separated ->death/reap checks, I am waiting for your
reply.
Signed-off-by: Oleg Nesterov <o...@redhat.com>
---
kernel/utrace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- kstub/kernel/utrace.c~5_get_lock_must_check_reap 2010-08-14
04:30:29.000000000 +0200
+++ kstub/kernel/utrace.c 2010-08-16 11:17:05.000000000 +0200
@@ -421,7 +421,7 @@ static struct utrace *get_utrace_lock(st
utrace = task_utrace_struct(target);
spin_lock(&utrace->lock);
- if (unlikely(!engine->ops) ||
+ if (unlikely(utrace->reap) || unlikely(!engine->ops) ||
unlikely(engine->ops == &utrace_detached_ops)) {
/*
* By the time we got the utrace lock,
