lkml.org 
[lkml]   [2010]   [Aug]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 2/2] CRED: Fix __task_cred()'s lockdep check and banner comment
    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.

    __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.

    > > fixed the lockdep checks on __task_cred().  This has shown up a place in the
    > > signalling code where a lock should be held - namely that
    > > check_kill_permission() requires its callers to hold the RCU lock.
    >
    > It's not just check_kill_permission(), is it? I thought we could do
    > the "for_each_process()" loops with just RCU, rather than holding the
    > whole tasklist_lock?

    Yes, for_each_process() is rcu-safe by itself.

    > So I _think_ that getting the RCU read-lock would
    > make it possible to get rid of the tasklist_lock in there too? At
    > least in kill_something_info().

    As for kill_something_info(), I think yes. I even sent (iirc) the
    protoptype patch a long ago. We can't just remove tasklist, we should
    avoid the races fork/exit/exec in the kill(-1, SIG) case.

    The same for kill_pgrp/__kill_pgrp_info(). We need tasklist to ensure
    that nobody in this group can escape the signal. This seems solveable
    too, it was even discussed a bit.

    > > It's may be that it would be better to add RCU read lock calls in
    > > group_send_sig_info() only, around the call to check_kill_permission().

    I must admit, at first glance changing check_kill_permission() to take
    rcu lock looks better 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() ?

    There are other examples of rcu_read_lock() under local_irq_disable().

    > > --- a/kernel/exit.c
    > > +++ b/kernel/exit.c
    > > @@ -773,6 +773,7 @@ static void forget_original_parent(struct task_struct *father)
    > >
    > >        exit_ptrace(father);
    > >
    > > +       rcu_read_lock();
    > >        write_lock_irq(&tasklist_lock);
    > >        reaper = find_new_reaper(father);

    No, this doesn't look right. find_new_reaper() can drop tasklist and sleep.

    Besides, this patch conflicts with the change in -mm tree. And imho this
    looks a bit as "action at a distance".

    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/
    \
     
     \ /
      Last update: 2010-08-04 15:23    [W:4.477 / U:0.128 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site