USB: serial: fix lifetime and locking problems

This patch (as1229) fixes a few lifetime and locking problems in the
usb-serial driver.  The main symptom is that an invalid kevent is
created when the serial device is unplugged while a connection is
active.

	Ports should be unregistered when device is disconnected,
	not when the parent usb_serial structure is deallocated.

	Each open file should hold a reference to the corresponding
	port structure, and the reference should be released when
	the file is closed.

	serial->disc_mutex should be acquired in serial_open(), to
	resolve the classic race between open and disconnect.

	serial_close() doesn't need to hold both serial->disc_mutex
	and port->mutex at the same time.

	Release the subdriver's module reference only after releasing
	all the other references, in case one of the release routines
	needs to invoke some code in the subdriver module.

	Replace a call to flush_scheduled_work() (which is prone to
	deadlocks) with cancel_work_sync().  Also, add a call to
	cancel_work_sync() in the disconnect routine.

	Reduce the scope of serial->disc_mutex in serial_disconnect().
	The only place it really needs to protect is where the
	"disconnected" flag is set.

This fixes the bug reported in

	http://bugs.freedesktop.org/show_bug.cgi?id=20703

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Tested-by: Dan Williams <dcbw@redhat.com>
Tested-by: Ming Lei <tom.leiming@gmail.com>
Reviewed-by: Oliver Neukum <oliver@neukum.org>
Acked-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index 2a70563..0a566ee 100644 (file)
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -137,22 +137,10 @@ static void destroy_serial(struct kref *kref)
 
        dbg("%s - %s", __func__, serial->type->description);
 
-       serial->type->shutdown(serial);
-
        /* return the minor range that this device had */
        if (serial->minor != SERIAL_TTY_NO_MINOR)
                return_serial(serial);
 
-       for (i = 0; i < serial->num_ports; ++i)
-               serial->port[i]->port.count = 0;
-
-       /* the ports are cleaned up and released in port_release() */
-       for (i = 0; i < serial->num_ports; ++i)
-               if (serial->port[i]->dev.parent != NULL) {
-                       device_unregister(&serial->port[i]->dev);
-                       serial->port[i] = NULL;
-               }
-
        /* If this is a "fake" port, we have to clean it up here, as it will
         * not get cleaned up in port_release() as it was never registered with
         * the driver core */
@@ -187,7 +175,7 @@ static int serial_open (struct tty_struct *tty, struct file *filp)
        struct usb_serial *serial;
        struct usb_serial_port *port;
        unsigned int portNumber;
-       int retval;
+       int retval = 0;
 
        dbg("%s", __func__);
 
@@ -198,21 +186,24 @@ static int serial_open (struct tty_struct *tty, struct file *filp)
                return -ENODEV;
        }
 
+       mutex_lock(&serial->disc_mutex);
        portNumber = tty->index - serial->minor;
        port = serial->port[portNumber];
-       if (!port) {
-               retval = -ENODEV;
-               goto bailout_kref_put;
-       }
-
-       if (port->serial->disconnected) {
+       if (!port || serial->disconnected)
                retval = -ENODEV;
-               goto bailout_kref_put;
-       }
+       else
+               get_device(&port->dev);
+       /*
+        * Note: Our locking order requirement does not allow port->mutex
+        * to be acquired while serial->disc_mutex is held.
+        */
+       mutex_unlock(&serial->disc_mutex);
+       if (retval)
+               goto bailout_serial_put;
 
        if (mutex_lock_interruptible(&port->mutex)) {
                retval = -ERESTARTSYS;
-               goto bailout_kref_put;
+               goto bailout_port_put;
        }
 
        ++port->port.count;
@@ -232,14 +223,20 @@ static int serial_open (struct tty_struct *tty, struct file *filp)
                        goto bailout_mutex_unlock;
                }
 
-               retval = usb_autopm_get_interface(serial->interface);
+               mutex_lock(&serial->disc_mutex);
+               if (serial->disconnected)
+                       retval = -ENODEV;
+               else
+                       retval = usb_autopm_get_interface(serial->interface);
                if (retval)
                        goto bailout_module_put;
+
                /* only call the device specific open if this
                 * is the first time the port is opened */
                retval = serial->type->open(tty, port, filp);
                if (retval)
                        goto bailout_interface_put;
+               mutex_unlock(&serial->disc_mutex);
        }
 
        mutex_unlock(&port->mutex);
@@ -248,13 +245,16 @@ static int serial_open (struct tty_struct *tty, struct file *filp)
 bailout_interface_put:
        usb_autopm_put_interface(serial->interface);
 bailout_module_put:
+       mutex_unlock(&serial->disc_mutex);
        module_put(serial->type->driver.owner);
 bailout_mutex_unlock:
        port->port.count = 0;
        tty->driver_data = NULL;
        tty_port_tty_set(&port->port, NULL);
        mutex_unlock(&port->mutex);
-bailout_kref_put:
+bailout_port_put:
+       put_device(&port->dev);
+bailout_serial_put:
        usb_serial_put(serial);
        return retval;
 }
@@ -262,6 +262,9 @@ bailout_kref_put:
 static void serial_close(struct tty_struct *tty, struct file *filp)
 {
        struct usb_serial_port *port = tty->driver_data;
+       struct usb_serial *serial;
+       struct module *owner;
+       int count;
 
        if (!port)
                return;
@@ -269,6 +272,8 @@ static void serial_close(struct tty_struct *tty, struct file *filp)
        dbg("%s - port %d", __func__, port->number);
 
        mutex_lock(&port->mutex);
+       serial = port->serial;
+       owner = serial->type->driver.owner;
 
        if (port->port.count == 0) {
                mutex_unlock(&port->mutex);
@@ -281,7 +286,7 @@ static void serial_close(struct tty_struct *tty, struct file *filp)
                 * this before we drop the port count. The call is protected
                 * by the port mutex
                 */
-               port->serial->type->close(tty, port, filp);
+               serial->type->close(tty, port, filp);
 
        if (port->port.count == (port->console ? 2 : 1)) {
                struct tty_struct *tty = tty_port_tty_get(&port->port);
@@ -295,17 +300,23 @@ static void serial_close(struct tty_struct *tty, struct file *filp)
                }
        }
 
-       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;
-
+       count = port->port.count;
        mutex_unlock(&port->mutex);
-       usb_serial_put(port->serial);
+       put_device(&port->dev);
+
+       /* Mustn't dereference port any more */
+       if (count == 0) {
+               mutex_lock(&serial->disc_mutex);
+               if (!serial->disconnected)
+                       usb_autopm_put_interface(serial->interface);
+               mutex_unlock(&serial->disc_mutex);
+       }
+       usb_serial_put(serial);
+
+       /* Mustn't dereference serial any more */
+       if (count == 0)
+               module_put(owner);
 }
 
 static int serial_write(struct tty_struct *tty, const unsigned char *buf,
@@ -549,7 +560,13 @@ static void kill_traffic(struct usb_serial_port *port)
 
 static void port_free(struct usb_serial_port *port)
 {
+       /*
+        * Stop all the traffic before cancelling the work, so that
+        * nobody will restart it by calling usb_serial_port_softint.
+        */
        kill_traffic(port);
+       cancel_work_sync(&port->work);
+
        usb_free_urb(port->read_urb);
        usb_free_urb(port->write_urb);
        usb_free_urb(port->interrupt_in_urb);
@@ -558,7 +575,6 @@ static void port_free(struct usb_serial_port *port)
        kfree(port->bulk_out_buffer);
        kfree(port->interrupt_in_buffer);
        kfree(port->interrupt_out_buffer);
-       flush_scheduled_work();         /* port->work */
        kfree(port);
 }
 
@@ -1043,6 +1059,12 @@ void usb_serial_disconnect(struct usb_interface *interface)
        usb_set_intfdata(interface, NULL);
        /* must set a flag, to signal subdrivers */
        serial->disconnected = 1;
+       mutex_unlock(&serial->disc_mutex);
+
+       /* Unfortunately, many of the sub-drivers expect the port structures
+        * to exist when their shutdown method is called, so we have to go
+        * through this awkward two-step unregistration procedure.
+        */
        for (i = 0; i < serial->num_ports; ++i) {
                port = serial->port[i];
                if (port) {
@@ -1052,11 +1074,21 @@ void usb_serial_disconnect(struct usb_interface *interface)
                                tty_kref_put(tty);
                        }
                        kill_traffic(port);
+                       cancel_work_sync(&port->work);
+                       device_del(&port->dev);
+               }
+       }
+       serial->type->shutdown(serial);
+       for (i = 0; i < serial->num_ports; ++i) {
+               port = serial->port[i];
+               if (port) {
+                       put_device(&port->dev);
+                       serial->port[i] = NULL;
                }
        }
+
        /* let the last holder of this object
         * cause it to be cleaned up */
-       mutex_unlock(&serial->disc_mutex);
        usb_serial_put(serial);
        dev_info(dev, "device disconnected\n");
 }