Messages in this thread | | | Date | Wed, 4 Aug 2010 17:08:05 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH 2/2] CRED: Fix __task_cred()'s lockdep check and banner comment |
| |
On 08/04, David Howells wrote: > > Oleg Nesterov <oleg@redhat.com> wrote: > > > On 08/03, Linus Torvalds wrote: > > > > > > On Tue, Aug 3, 2010 at 2:34 AM, David Howells <dhowells@redhat.com> wrote: > > > > > > > > A previous patch: > > > > > > > > commit 8f92054e7ca1d3a3ae50fb42d2253ac8730d9b2a > > > > Author: David Howells <dhowells@redhat.com> > > > > Date: Thu Jul 29 12:45:55 2010 +0100 > > > > Subject: CRED: Fix __task_cred()'s lockdep check and banner comment > > > > I am not sure I understand this patch. > > You are talking about the 'previous patch'? > > > __task_cred() checks rcu_read_lock_held() || task_is_dead(), and > > task_is_dead(task) is ((task)->exit_state != 0). > > > > OK, task_is_dead() is valid for, say, wait_task_zombie(). But > > wait_task_stopped() calls __task_cred(p) without rcu lock and p is alive. > > The code is correct, this thread can do nothing until we drop ->siglock. > > The problem is that we have to tell lockdep this. Just checking in > __task_cred() that siglock is held is insufficient. That doesn't handle, say, > sys_setuid() from changing the credentials, and effectively skips the check in > places where it mustn't. > > Similarly, having interrupts disabled on the CPU we're running on doesn't help > either, since it doesn't stop another CPU replacing those credentials. > > There are ways of dealing with wait_task_stopped(): > > (1) Place an rcu_read_lock()'d section around the call to __task_cred().
Sure, this solves the problem. But probably this needs a comment to explain why do we take rcu lock.
OTOH, wait_task_continued() does need rcu_read_lock(), the task is running.
UNLESS we believe that local_irq_disable() makes rcu_read_lock() unnecessary, see below.
> (2) Make __task_cred()'s lockdep understand about the target task being > stopped whilst we hold its siglock.
May be... but we have so many special cases. Say, fill_psinfo()->__task_cred(). This is called under rcu lock, but it is not needed. The task is either current or it sleeps in exit_mm().
I mean, perhaps it is better to either always require rcu_read_lock() around __task_cred() even if it is not needed, or do not use rcu_dereference_check() at all.
In any case, task_is_dead() doesn't help afaics, it is only useful for wait_task_zombie().
> > I must admit, at first glance changing check_kill_permission() to take > > rcu lock looks better to me. > > I think group_send_sig_info() would be better. The only other caller of > c_k_p() already has to hold the RCU read lock for other reasons. > > How about the attached patch then?
Agreed, the patch looks fine to me.
> > > > On the other hand, some of the callers are either holding the RCU read > > > > lock already, or have disabled interrupts, > > > > Hmm. So, local_irq_disable() "officially" blocks rcu? It does in practice > > (unless I missed the new version of RCU), but, say, posix_timer_event() > > takes rcu_read_lock() exactly because I thought we shouldn't assume that > > irqs_disabled() acts as rcu_read_lock() ? > > This CPU can't be preempted if it can't be interrupted, I think.
Yes, please note "It does in practice" above.
My question is, should/can we rely on this fact? Or should we assume that nothing except rcu_read_lock() implies rcu_read_lock() ?
Oleg.
-- 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/
| |