[PATCH] Bluetooth: fix oops in rfcomm tty code (v2) - Kernel

This is a discussion on [PATCH] Bluetooth: fix oops in rfcomm tty code (v2) - Kernel ; Hi, This is a resend of a patch I sent earlier. It fixes a reproducible oops (see http://lkml.org/lkml/2008/7/13/89 for test case), and I'd be very happy for some feedback from Bluetooth people. Can this be included for testing somewhere? I ...

+ Reply to Thread
Results 1 to 4 of 4

Thread: [PATCH] Bluetooth: fix oops in rfcomm tty code (v2)

  1. [PATCH] Bluetooth: fix oops in rfcomm tty code (v2)

    Hi,

    This is a resend of a patch I sent earlier. It fixes a reproducible
    oops (see http://lkml.org/lkml/2008/7/13/89 for test case), and I'd
    be very happy for some feedback from Bluetooth people. Can this be
    included for testing somewhere? I don't have any bluetooth devices
    myself, so all my testing is limited to creating/releasing devices
    with ioctl (I'm not sure that's good enough).

    Dave: I have extended the rfcomm_dev_lock to include all the setup and
    teardown of a single device. On second thought, it doesn't really make
    sense to use a separate lock for that. May I have your opinion on this
    second version? (I've fixed the comments/BUG_ON that you pointed out.)


    Vegard


    From ee99cfaae05bf6173efc77e5a100f1fbbc668301 Mon Sep 17 00:00:00 2001
    From: Vegard Nossum
    Date: Sun, 13 Jul 2008 19:02:11 +0200
    Subject: [PATCH] Bluetooth: fix oops in rfcomm tty code

    Soeren Sonnenburg reported:
    > this oops happened after a couple of s2ram cycles so it might be very
    > well crap. However I somehow triggered it by /etc/init.d/bluetooth
    > stop/start's which also call hid2hci maybe even a connection was about
    > to be established at that time. As I remember having seen a problem like
    > this before I thought I report it (even though I have a madwifi tainted
    > kernel).
    >
    > kobject_add_internal failed for rfcomm0 with -EEXIST, don't try to register things with the same name in the same directory.


    It turns out that the following sequence of actions will reproduce the
    oops:

    1. Create a new rfcomm device (using RFCOMMCREATEDEV ioctl)
    2. (Try to) open the device
    3. Release the rfcomm device (using RFCOMMRELEASEDEV ioctl)

    At this point, the "rfcomm?" tty is still in use, but the device is gone
    from the internal rfcomm list, so the device id can be reused.

    4. Create a new rfcomm device with the same device id as before

    And now kobject will complain that the tty already exists.

    (See http://lkml.org/lkml/2008/7/13/89 for a reproducible test-case.)

    This patch attempts to correct this by only removing the device from the
    internal rfcomm list of devices at the final unregister, so that the id
    won't get reused until the device has been completely destructed.

    This should be safe as the RFCOMM_TTY_RELEASED bit will be set for the
    device and prevent the device from being reopened after it has been
    released.

    We also fix a race (which would lead to the same oops) by including the
    tty setup/teardown code in the rfcomm_dev_lock, the lock protecting the
    list of devices.

    Thanks to Dave Young for additional suggestions.

    Reported-by: Soeren Sonnenburg
    Cc: Marcel Holtmann
    Cc: Maxim Krasnyansky
    Cc: David Woodhouse
    Cc: Dave Young
    Signed-off-by: Vegard Nossum
    ---
    net/bluetooth/rfcomm/tty.c | 27 ++++++++++++++-------------
    1 files changed, 14 insertions(+), 13 deletions(-)

    diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
    index c919187..1709ccf 100644
    --- a/net/bluetooth/rfcomm/tty.c
    +++ b/net/bluetooth/rfcomm/tty.c
    @@ -96,9 +96,11 @@ static void rfcomm_dev_destruct(struct rfcomm_dev *dev)
    BT_DBG("dev %p dlc %p", dev, dlc);

    /* Refcount should only hit zero when called from rfcomm_dev_del()
    - which will have taken us off the list. Everything else are
    - refcounting bugs. */
    - BUG_ON(!list_empty(&dev->list));
    + which will have set the RFCOMM_TTY_RELEASED bit. Everything else
    + are refcounting bugs. */
    + BUG_ON(!test_bit(RFCOMM_TTY_RELEASED, &dev->flags));
    +
    + write_lock_bh(&rfcomm_dev_lock);

    rfcomm_dlc_lock(dlc);
    /* Detach DLC if it's owned by this dev */
    @@ -108,8 +110,11 @@ static void rfcomm_dev_destruct(struct rfcomm_dev *dev)

    rfcomm_dlc_put(dlc);

    + list_del_init(&dev->list);
    tty_unregister_device(rfcomm_tty_driver, dev->id);

    + write_unlock_bh(&rfcomm_dev_lock);
    +
    kfree(dev);

    /* It's safe to call module_put() here because socket still
    @@ -127,10 +132,8 @@ static inline void rfcomm_dev_put(struct rfcomm_dev *dev)
    /* The reason this isn't actually a race, as you no
    doubt have a little voice screaming at you in your
    head, is that the refcount should never actually
    - reach zero unless the device has already been taken
    - off the list, in rfcomm_dev_del(). And if that's not
    - true, we'll hit the BUG() in rfcomm_dev_destruct()
    - anyway. */
    + reach zero unless we've already set the
    + RFCOMM_TTY_RELEASED bit in rfcomm_dev_del(). */
    if (atomic_dec_and_test(&dev->refcnt))
    rfcomm_dev_destruct(dev);
    }
    @@ -278,9 +281,8 @@ static int rfcomm_dev_add(struct rfcomm_dev_req *req, struct rfcomm_dlc *dlc)
    __module_get(THIS_MODULE);

    out:
    - write_unlock_bh(&rfcomm_dev_lock);
    -
    if (err < 0) {
    + write_unlock_bh(&rfcomm_dev_lock);
    kfree(dev);
    return err;
    }
    @@ -290,6 +292,7 @@ out:
    if (IS_ERR(dev->tty_dev)) {
    err = PTR_ERR(dev->tty_dev);
    list_del(&dev->list);
    + write_unlock_bh(&rfcomm_dev_lock);
    kfree(dev);
    return err;
    }
    @@ -302,6 +305,8 @@ out:
    if (device_create_file(dev->tty_dev, &dev_attr_channel) < 0)
    BT_ERR("Failed to create channel attribute");

    + write_unlock_bh(&rfcomm_dev_lock);
    +
    return dev->id;
    }

    @@ -314,10 +319,6 @@ static void rfcomm_dev_del(struct rfcomm_dev *dev)
    else
    set_bit(RFCOMM_TTY_RELEASED, &dev->flags);

    - write_lock_bh(&rfcomm_dev_lock);
    - list_del_init(&dev->list);
    - write_unlock_bh(&rfcomm_dev_lock);
    -
    rfcomm_dev_put(dev);
    }

    --
    1.5.5.1

    --
    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] Bluetooth: fix oops in rfcomm tty code (v2)

    Hi Vegard,

    > This is a resend of a patch I sent earlier. It fixes a reproducible
    > oops (see http://lkml.org/lkml/2008/7/13/89 for test case), and I'd
    > be very happy for some feedback from Bluetooth people. Can this be
    > included for testing somewhere? I don't have any bluetooth devices
    > myself, so all my testing is limited to creating/releasing devices
    > with ioctl (I'm not sure that's good enough).
    >
    > Dave: I have extended the rfcomm_dev_lock to include all the setup and
    > teardown of a single device. On second thought, it doesn't really make
    > sense to use a separate lock for that. May I have your opinion on this
    > second version? (I've fixed the comments/BUG_ON that you pointed out.)


    it seems it is the actually the first time, I see this one. Anyway, so
    holding that lock is a bad idea. Fixing this the right way might be
    tricky since the TTY layer is involved here and own the kobject. So I
    would say we have to make sure that the RFCOMM TTY will hangup when
    calling RELEASEDEV or otherwise fail.

    Regards

    Marcel


    --
    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] Bluetooth: fix oops in rfcomm tty code (v2)

    On Sat, Jul 19, 2008 at 11:38 PM, Marcel Holtmann wrote:
    > Hi Vegard,
    >
    >> This is a resend of a patch I sent earlier. It fixes a reproducible
    >> oops (see http://lkml.org/lkml/2008/7/13/89 for test case), and I'd
    >> be very happy for some feedback from Bluetooth people. Can this be
    >> included for testing somewhere? I don't have any bluetooth devices
    >> myself, so all my testing is limited to creating/releasing devices
    >> with ioctl (I'm not sure that's good enough).
    >>
    >> Dave: I have extended the rfcomm_dev_lock to include all the setup and
    >> teardown of a single device. On second thought, it doesn't really make
    >> sense to use a separate lock for that. May I have your opinion on this
    >> second version? (I've fixed the comments/BUG_ON that you pointed out.)

    >
    > it seems it is the actually the first time, I see this one. Anyway, so
    > holding that lock is a bad idea. Fixing this the right way might be
    > tricky since the TTY layer is involved here and own the kobject. So I
    > would say we have to make sure that the RFCOMM TTY will hangup when
    > calling RELEASEDEV or otherwise fail.


    On Mon, Jul 21, 2008 at 7:14 AM, Marcel Holtmann wrote:
    >> The patch titled
    >> Bluetooth: fix oops in rfcomm tty code
    >> has been added to the -mm tree. Its filename is
    >> bluetooth-fix-oops-in-rfcomm-tty-code-v2.patch
    >>

    > just a quick note that I am not okay with this patch. Holding the lock
    > isn't the right solution since it would also block any other application
    > from creating new devices. We can't do this.


    Hi,

    We are not holding the lock across any ioctl/syscall boundary, which
    would be an error anyway.

    The lock now additionally protects the calls to:

    tty_register_device
    tty_unregister_device
    device_create_file

    I don't think these functions block or do anything with the device
    apart from just registering/unregistering the files.

    Can you please explain in more detail what is wrong with the patch?
    Where are we preventing other applications from creating new devices?
    My intention was to prevent other applications from creating the
    _same_ devices while they are still in use, although attempting to
    register a new device (with an already-in-use ID) should simply fail,
    and not block.

    Thank you,


    Vegard

    --
    "The animistic metaphor of the bug that maliciously sneaked in while
    the programmer was not looking is intellectually dishonest as it
    disguises that the error is the programmer's own creation."
    -- E. W. Dijkstra, EWD1036



    --
    "The animistic metaphor of the bug that maliciously sneaked in while
    the programmer was not looking is intellectually dishonest as it
    disguises that the error is the programmer's own creation."
    -- E. W. Dijkstra, EWD1036
    --
    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] Bluetooth: fix oops in rfcomm tty code (v2)

    Hi Vegard,

    > >> This is a resend of a patch I sent earlier. It fixes a reproducible
    > >> oops (see http://lkml.org/lkml/2008/7/13/89 for test case), and I'd
    > >> be very happy for some feedback from Bluetooth people. Can this be
    > >> included for testing somewhere? I don't have any bluetooth devices
    > >> myself, so all my testing is limited to creating/releasing devices
    > >> with ioctl (I'm not sure that's good enough).
    > >>
    > >> Dave: I have extended the rfcomm_dev_lock to include all the setup and
    > >> teardown of a single device. On second thought, it doesn't really make
    > >> sense to use a separate lock for that. May I have your opinion on this
    > >> second version? (I've fixed the comments/BUG_ON that you pointed out.)

    > >
    > > it seems it is the actually the first time, I see this one. Anyway, so
    > > holding that lock is a bad idea. Fixing this the right way might be
    > > tricky since the TTY layer is involved here and own the kobject. So I
    > > would say we have to make sure that the RFCOMM TTY will hangup when
    > > calling RELEASEDEV or otherwise fail.

    >
    > On Mon, Jul 21, 2008 at 7:14 AM, Marcel Holtmann wrote:
    > >> The patch titled
    > >> Bluetooth: fix oops in rfcomm tty code
    > >> has been added to the -mm tree. Its filename is
    > >> bluetooth-fix-oops-in-rfcomm-tty-code-v2.patch
    > >>

    > > just a quick note that I am not okay with this patch. Holding the lock
    > > isn't the right solution since it would also block any other application
    > > from creating new devices. We can't do this.

    >
    > Hi,
    >
    > We are not holding the lock across any ioctl/syscall boundary, which
    > would be an error anyway.
    >
    > The lock now additionally protects the calls to:
    >
    > tty_register_device
    > tty_unregister_device
    > device_create_file
    >
    > I don't think these functions block or do anything with the device
    > apart from just registering/unregistering the files.
    >
    > Can you please explain in more detail what is wrong with the patch?
    > Where are we preventing other applications from creating new devices?
    > My intention was to prevent other applications from creating the
    > _same_ devices while they are still in use, although attempting to
    > register a new device (with an already-in-use ID) should simply fail,
    > and not block.


    I see an issue when a malicious application like the test program keeps
    the TTY open. Then we would block any other TTY creation. Even if it is
    a different TTY or process.

    Regards

    Marcel


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