futex: futex_find_get_task make credentials check conditional
in [Kernel]
|
Prev: [PATCH 0/2] padata: Separate cpumasks for cb_cpus and parallel workers
Next: [PATCH 2/2] pcrypt: sysfs interface
From: Linus Torvalds on 29 Jun 2010 12:50 On Tue, Jun 29, 2010 at 1:42 AM, Michal Hocko <mhocko(a)suse.cz> wrote: > > futex_find_get_task is currently used (through lookup_pi_state) from two > contexts, futex_requeue and futex_lock_pi_atomic. While credentials check > makes sense in the first code path, the second one is more problematic > because this check requires that the PI lock holder (pid parameter) has > the same uid and euid as the process's euid which is trying to lock the > same futex (current). So exactly why does it make sense to check the credentials in the first code path then? Shouldn't the futex issue in the end depend on whether you have a shared page or not - and not on credentials at all? Any two processes that share a futex in the same shared page should be able to use that without any regard for whether they are the same user. That's kind of the point, no? IOW, I personally dislike these kinds of conditional checks, especially since the discussion (at least the part I've seen) hasn't made it clear why it should be conditional - or exist - in the first place. So I'd like the patch to include an explanation of exactly why the two cases are different. The other thing I'd like to see is to move the whole cred checking up a level. There's no reason to check the credentials in futex_find_get_task() that I can see - why not do it in the caller instead? IOW, I think futex_find_get_task() should just look something like this instead: static struct task_struct * futex_find_get_task(pid_t pid) { struct task_struct *p; rcu_read_lock(); p = find_task_by_vpid(pid); if (p) get_task_struct(p); rcu_read_unlock(); return p; } and then in the caller we'd do something like p = futex_find_get_task(pid); if (!p) return -ESEARCH; if ( .. check p credentials is necessary and fails..) goto put_task_and_exit; because especially with not everybody needing the credentials check, I do not think it should be done at the lowest level (it's clearly not fundamental to the operation, so it shouldn't be part of the core lookup). With some re-factoring, it might even be possible to avoid a dynamic check at all, and just have two different static paths for the two cases. That's a separate issue, though. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |