lkml.org 
[lkml]   [2010]   [Feb]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    Subject[05/93] Fix race in tty_fasync() properly
    2.6.32-stable review patch.  If anyone has any objections, please let us know.

    ------------------

    From: Linus Torvalds <torvalds@linux-foundation.org>

    commit 80e1e823989ec44d8e35bdfddadbddcffec90424 upstream.

    This reverts commit 703625118069 ("tty: fix race in tty_fasync") and
    commit b04da8bfdfbb ("fnctl: f_modown should call write_lock_irqsave/
    restore") that tried to fix up some of the fallout but was incomplete.

    It turns out that we really cannot hold 'tty->ctrl_lock' over calling
    __f_setown, because not only did that cause problems with interrupt
    disables (which the second commit fixed), it also causes a potential
    ABBA deadlock due to lock ordering.

    Thanks to Tetsuo Handa for following up on the issue, and running
    lockdep to show the problem. It goes roughly like this:

    - f_getown gets filp->f_owner.lock for reading without interrupts
    disabled, so an interrupt that happens while that lock is held can
    cause a lockdep chain from f_owner.lock -> sighand->siglock.

    - at the same time, the tty->ctrl_lock -> f_owner.lock chain that
    commit 703625118069 introduced, together with the pre-existing
    sighand->siglock -> tty->ctrl_lock chain means that we have a lock
    dependency the other way too.

    So instead of extending tty->ctrl_lock over the whole __f_setown() call,
    we now just take a reference to the 'pid' structure while holding the
    lock, and then release it after having done the __f_setown. That still
    guarantees that 'struct pid' won't go away from under us, which is all
    we really ever needed.

    Reported-and-tested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
    Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
    Acked-by: Américo Wang <xiyou.wangcong@gmail.com>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
    Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

    ---
    drivers/char/tty_io.c | 4 +++-
    fs/fcntl.c | 6 ++----
    2 files changed, 5 insertions(+), 5 deletions(-)

    --- a/drivers/char/tty_io.c
    +++ b/drivers/char/tty_io.c
    @@ -1930,8 +1930,10 @@ static int tty_fasync(int fd, struct fil
    pid = task_pid(current);
    type = PIDTYPE_PID;
    }
    - retval = __f_setown(filp, pid, type, 0);
    + get_pid(pid);
    spin_unlock_irqrestore(&tty->ctrl_lock, flags);
    + retval = __f_setown(filp, pid, type, 0);
    + put_pid(pid);
    if (retval)
    goto out;
    } else {
    --- a/fs/fcntl.c
    +++ b/fs/fcntl.c
    @@ -199,9 +199,7 @@ static int setfl(int fd, struct file * f
    static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
    int force)
    {
    - unsigned long flags;
    -
    - write_lock_irqsave(&filp->f_owner.lock, flags);
    + write_lock_irq(&filp->f_owner.lock);
    if (force || !filp->f_owner.pid) {
    put_pid(filp->f_owner.pid);
    filp->f_owner.pid = get_pid(pid);
    @@ -213,7 +211,7 @@ static void f_modown(struct file *filp,
    filp->f_owner.euid = cred->euid;
    }
    }
    - write_unlock_irqrestore(&filp->f_owner.lock, flags);
    + write_unlock_irq(&filp->f_owner.lock);
    }

    int __f_setown(struct file *filp, struct pid *pid, enum pid_type type,

    --
    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-02-19 18:03    [W:0.029 / U:60.776 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site