[PATCH] Fix race/oops in tty layer after BKL pushdown - Kernel

This is a discussion on [PATCH] Fix race/oops in tty layer after BKL pushdown - Kernel ; Hello Alan, while testing our KVM code for s390 (starting and killall kvm in a loop) I can reproduce the following oops: Unable to handle kernel pointer dereference at virtual kernel address 6b6b6b6b6b6b6000 Oops: 0038 [#1] SMP Modules linked in: ...

+ Reply to Thread
Results 1 to 4 of 4

Thread: [PATCH] Fix race/oops in tty layer after BKL pushdown

  1. [PATCH] Fix race/oops in tty layer after BKL pushdown

    Hello Alan,

    while testing our KVM code for s390 (starting and killall kvm in a loop) I
    can reproduce the following oops:

    Unable to handle kernel pointer dereference at virtual kernel address 6b6b6b6b6b6b6000
    Oops: 0038 [#1] SMP
    Modules linked in: dm_multipath sunrpc qeth_l3 qeth_l2 dm_mod qeth ccwgroup
    CPU: 1 Not tainted 2.6.27-rc1 #54
    Process kuli (pid: 4409, task: 00000000b6aa5940, ksp: 00000000b7343e10)
    Krnl PSW : 0704e00180000000 00000000002e0b8c (disassociate_ctty+0x1c0/0x288)
    R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 PM:0 EA:3
    Krnl GPRS: 0000000000000000 6b6b6b6b6b6b6b6b 0000000000000001 00000000000003a6
    00000000002e0a46 00000000004b4160 0000000000000001 00000000bbd79758
    00000000b7343e58 00000000b8854148 00000000bd34dea0 00000000b7343c20
    0000000000000001 00000000004b6d08 00000000002e0a46 00000000b7343c20
    Krnl Code: 00000000002e0b7e: eb9fb0a00004 lmg %r9,%r15,160(%r11)
    00000000002e0b84: 07f4 bcr 15,%r4
    00000000002e0b86: e31090080004 lg %r1,8(%r9)
    >00000000002e0b8c: d501109cd000 clc 156(2,%r1),0(%r13)

    00000000002e0b92: a784ff5d brc 8,2e0a4c
    00000000002e0b96: b9040029 lgr %r2,%r9
    00000000002e0b9a: c0e5fffff9c3 brasl %r14,2dff20
    00000000002e0ba0: a7f4ff56 brc 15,2e0a4c
    Call Trace:
    ([<00000000002e0a46>] disassociate_ctty+0x7a/0x288)
    [<0000000000141fe6>] do_exit+0x212/0x8d4
    [<0000000000142708>] do_group_exit+0x60/0xcc
    [<0000000000150660>] get_signal_to_deliver+0x270/0x3ac
    [<000000000010bfd6>] do_signal+0x8e/0x8dc
    [<0000000000113772>] sysc_sigpending+0xe/0x22
    [<000001ff0000b134>] 0x1ff0000b134
    INFO: lockdep is turned off.
    Last Breaking-Event-Address:
    [<00000000002e0a48>] disassociate_ctty+0x7c/0x288
    <0>Kernel panic - not syncing: Fatal exception: panic_on_oops

    It seems that tty was already free in disassocate_ctty when it tries
    to dereference tty->driver.

    After moving the lock_kernel before the mutex_unlock, I can no longer
    reproduce the problem.

    Please review and consider to apply:

    Signed-off-by: Christian Borntraeger
    ---
    drivers/char/tty_io.c | 2 +-
    1 file changed, 1 insertion(+), 1 deletion(-)

    Index: kvm/drivers/char/tty_io.c
    ================================================== =================
    --- kvm.orig/drivers/char/tty_io.c
    +++ kvm/drivers/char/tty_io.c
    @@ -1161,8 +1161,8 @@ void disassociate_ctty(int on_exit)
    tty = get_current_tty();
    if (tty) {
    tty_pgrp = get_pid(tty->pgrp);
    - mutex_unlock(&tty_mutex);
    lock_kernel();
    + mutex_unlock(&tty_mutex);
    /* XXX: here we race, there is nothing protecting tty */
    if (on_exit && tty->driver->type != TTY_DRIVER_TYPE_PTY)
    tty_vhangup(tty);
    --
    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/

  2. Re: [PATCH] Fix race/oops in tty layer after BKL pushdown

    > It seems that tty was already free in disassocate_ctty when it tries
    > to dereference tty->driver.
    >
    > After moving the lock_kernel before the mutex_unlock, I can no longer
    > reproduce the problem.
    >
    > Please review and consider to apply:


    This doesn't help as the BKL doesn't protect tty here - we don't have
    refcounting on the ttys yet so this bug (which goes back forever) simply
    becomes a different sized race if you swap the lock ordering.

    Given tty_vhangup just fires off a wait queue which is killed on the tty
    destruction you should be able for now at least to move the mutex_unlock
    to after the call to tty_vhangup().

    Does that also fix the problem ?

    Alan
    --
    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/

  3. Re: [PATCH] Fix race/oops in tty layer after BKL pushdown

    Am Donnerstag, 7. August 2008 schrieb Alan Cox:
    > > It seems that tty was already free in disassocate_ctty when it tries
    > > to dereference tty->driver.
    > >
    > > After moving the lock_kernel before the mutex_unlock, I can no longer
    > > reproduce the problem.
    > >
    > > Please review and consider to apply:

    >
    > This doesn't help as the BKL doesn't protect tty here - we don't have
    > refcounting on the ttys yet so this bug (which goes back forever) simply
    > becomes a different sized race if you swap the lock ordering.
    >
    > Given tty_vhangup just fires off a wait queue which is killed on the tty
    > destruction you should be able for now at least to move the mutex_unlock
    > to after the call to tty_vhangup().
    >
    > Does that also fix the problem ?


    You mean something like the below patch? Yes, that works as well.

    Signed-off-by: Christian Borntraeger

    ---
    drivers/char/tty_io.c | 3 +--
    1 file changed, 1 insertion(+), 2 deletions(-)

    Index: kvm/drivers/char/tty_io.c
    ================================================== =================
    --- kvm.orig/drivers/char/tty_io.c
    +++ kvm/drivers/char/tty_io.c
    @@ -1161,12 +1161,11 @@ void disassociate_ctty(int on_exit)
    tty = get_current_tty();
    if (tty) {
    tty_pgrp = get_pid(tty->pgrp);
    - mutex_unlock(&tty_mutex);
    lock_kernel();
    - /* XXX: here we race, there is nothing protecting tty */
    if (on_exit && tty->driver->type != TTY_DRIVER_TYPE_PTY)
    tty_vhangup(tty);
    unlock_kernel();
    + mutex_unlock(&tty_mutex);
    } else if (on_exit) {
    struct pid *old_pgrp;
    spin_lock_irq(&current->sighand->siglock);
    --
    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/

  4. Re: [PATCH] Fix race/oops in tty layer after BKL pushdown

    > Please review and consider to apply:
    >
    > Signed-off-by: Christian Borntraeger
    > ---
    > drivers/char/tty_io.c | 2 +-
    > 1 file changed, 1 insertion(+), 1 deletion(-)
    >
    > Index: kvm/drivers/char/tty_io.c
    > ================================================== =================
    > --- kvm.orig/drivers/char/tty_io.c
    > +++ kvm/drivers/char/tty_io.c
    > @@ -1161,8 +1161,8 @@ void disassociate_ctty(int on_exit)
    > tty = get_current_tty();
    > if (tty) {
    > tty_pgrp = get_pid(tty->pgrp);
    > - mutex_unlock(&tty_mutex);
    > lock_kernel();
    > + mutex_unlock(&tty_mutex);
    > /* XXX: here we race, there is nothing protecting tty */
    > if (on_exit && tty->driver->type != TTY_DRIVER_TYPE_PTY)
    > tty_vhangup(tty);


    For upstream (see -next tree) I have pushed a refcounting solution (or at
    least I hope its a solution) to the ttydev tree. For 2.6.26 your approach
    partially fixes this and certainly doesn't make it worse so I'll now push
    it onwards.

    Alan
    --
    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/

+ Reply to Thread