Messages in this thread | | | From | Linus Torvalds <> | Date | Tue, 27 Jul 2010 10:56:20 -0700 | Subject | Re: [PATCH] cred - synchronize rcu before releasing cred |
| |
On Tue, Jul 27, 2010 at 9:46 AM, David Howells <dhowells@redhat.com> wrote: > > That's not the problem. > > The problem is that task_state() accesses the target task's credentials whilst > only holding the RCU read lock. That means that the existence of the cred > struct so accessed can only be guaranteed up to the point that the RCU read > lock is released.
Umm. In that case, get_task_cred() is actively misleading.
What you are saying is that you cannot do
rcu_read_lock() __cred = (struct cred *) __task_cred((task)); get_cred(__cred); rcu_read_unlock();
but that is _exactly_ what get_task_cred() does. And that __task_cred() check checks that we have
rcu_read_lock_held() || lockdep_tasklist_lock_is_held()
and what you are describing would require us to have a '&&' rather than a '||' in that test. Because it is _not_ sufficient to have just the rcu_read_lock held.
So it looks like the validation is simply wrong. The __task_cred() helper is buggy. It's used for two different cases, and they have totally different locking requirements.
Case #1: - you can do __task_cred() with just read-lock held, but then you cannot add refs to it
Case #2: - you can do __task_cred() with read-lock held _and_ guaranteeing that the task doesn't go away, and then you can hold a ref to it as long as you still guarantee the task is around.
And the comments are actively wrong. The comments talk about the "case #2" thing only. Ignoring case #1, except for the fact that the _check_ allows case #1, so you never get a warning from the RCU "proving" code even for incorrect code.
So presumably Jiri's patch is correct, but the reason the bug happened in the first place is that all those accessor functions are totally confused about how they supposed to be used, with incorrect comments and incorrect access checks.
That should get fixed. Who knows how many other buggy users there are due to the confusion?
Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |