[PATCH] tty: Fix close races in USB serial - Kernel

This is a discussion on [PATCH] tty: Fix close races in USB serial - Kernel ; From: Alan Cox USB serial has always had races where the tty port usage count can hit zero during a receive event. The internal locking is a mutex so we can't use that in the IRQ handlers. With krefs we ...

+ Reply to Thread
Results 1 to 4 of 4

Thread: [PATCH] tty: Fix close races in USB serial

  1. [PATCH] tty: Fix close races in USB serial

    From: Alan Cox

    USB serial has always had races where the tty port usage count can hit zero
    during a receive event. The internal locking is a mutex so we can't use
    that in the IRQ handlers.

    With krefs we can tackle this differently but we still need to be careful.

    Signed-off-by: Alan Cox
    ---

    drivers/usb/serial/usb-serial.c | 15 ++++++++++-----
    1 files changed, 10 insertions(+), 5 deletions(-)


    diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
    index 794b5ff..aafa684 100644
    --- a/drivers/usb/serial/usb-serial.c
    +++ b/drivers/usb/serial/usb-serial.c
    @@ -269,15 +269,19 @@ static void serial_close(struct tty_struct *tty, struct file *filp)
    return;
    }

    - --port->port.count;
    - if (port->port.count == 0)
    + if (port->port.count == 1)
    /* only call the device specific close if this
    - * port is being closed by the last owner */
    + * port is being closed by the last owner. Ensure we do
    + * this before we drop the port count. The call is protected
    + * by the port mutex
    + */
    port->serial->type->close(tty, port, filp);

    - if (port->port.count == (port->console? 1 : 0)) {
    + if (port->port.count == (port->console ? 2 : 1)) {
    struct tty_struct *tty = tty_port_tty_get(&port->port);
    if (tty) {
    + /* We must do this before we drop the port count to
    + zero. */
    if (tty->driver_data)
    tty->driver_data = NULL;
    tty_port_tty_set(&port->port, NULL);
    @@ -285,13 +289,14 @@ static void serial_close(struct tty_struct *tty, struct file *filp)
    }
    }

    - if (port->port.count == 0) {
    + if (port->port.count == 1) {
    mutex_lock(&port->serial->disc_mutex);
    if (!port->serial->disconnected)
    usb_autopm_put_interface(port->serial->interface);
    mutex_unlock(&port->serial->disc_mutex);
    module_put(port->serial->type->driver.owner);
    }
    + --port->port.count;

    mutex_unlock(&port->mutex);
    usb_serial_put(port->serial);

    --
    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] tty: Fix close races in USB serial

    On Tue, Nov 04, 2008 at 03:29:24PM +0000, Alan Cox wrote:
    > From: Alan Cox
    >
    > USB serial has always had races where the tty port usage count can hit zero
    > during a receive event. The internal locking is a mutex so we can't use
    > that in the IRQ handlers.
    >
    > With krefs we can tackle this differently but we still need to be careful.
    >
    > Signed-off-by: Alan Cox
    > ---
    >
    > drivers/usb/serial/usb-serial.c | 15 ++++++++++-----
    > 1 files changed, 10 insertions(+), 5 deletions(-)
    >
    >
    > diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
    > index 794b5ff..aafa684 100644
    > --- a/drivers/usb/serial/usb-serial.c
    > +++ b/drivers/usb/serial/usb-serial.c
    > @@ -269,15 +269,19 @@ static void serial_close(struct tty_struct *tty, struct file *filp)
    > return;
    > }
    >
    > - --port->port.count;
    > - if (port->port.count == 0)
    > + if (port->port.count == 1)
    > /* only call the device specific close if this
    > - * port is being closed by the last owner */
    > + * port is being closed by the last owner. Ensure we do
    > + * this before we drop the port count. The call is protected
    > + * by the port mutex
    > + */
    > port->serial->type->close(tty, port, filp);
    >
    > - if (port->port.count == (port->console? 1 : 0)) {
    > + if (port->port.count == (port->console ? 2 : 1)) {


    Why are you testing for 2 here?

    confused,

    greg k-h
    --
    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] tty: Fix close races in USB serial

    > > - if (port->port.count == (port->console? 1 : 0)) {
    > > + if (port->port.count == (port->console ? 2 : 1)) {

    >
    > Why are you testing for 2 here?


    At the point we enter this routine port->port.count is the number of
    users of the port. If this is the final close then this is 1 (us), except
    if it is the USB console (in which case there will be two references).

    The old code dropped the reference count first, the new code drops the
    kref and port->tty first, then shuts down the port and then drops the
    refcount. That avoids the race where we get the usb serial drivers trying
    to stuff bytes into a closed port and the ldisc layer in turn trying to
    echo them
    - which causes the warnings.

    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/

  4. Re: [PATCH] tty: Fix close races in USB serial

    On Tue, Nov 04, 2008 at 05:37:26PM +0000, Alan Cox wrote:
    > > > - if (port->port.count == (port->console? 1 : 0)) {
    > > > + if (port->port.count == (port->console ? 2 : 1)) {

    > >
    > > Why are you testing for 2 here?

    >
    > At the point we enter this routine port->port.count is the number of
    > users of the port. If this is the final close then this is 1 (us), except
    > if it is the USB console (in which case there will be two references).
    >
    > The old code dropped the reference count first, the new code drops the
    > kref and port->tty first, then shuts down the port and then drops the
    > refcount. That avoids the race where we get the usb serial drivers trying
    > to stuff bytes into a closed port and the ldisc layer in turn trying to
    > echo them
    > - which causes the warnings.


    Ah, ok, that makes sense, thanks.

    You might want to document that in there so that others understand it as
    well.

    thanks,

    greg k-h
    --
    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