Commit 2d93148a authored by Alan Stern's avatar Alan Stern Committed by Greg Kroah-Hartman
Browse files

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: default avatarAlan Stern <stern@rowland.harvard.edu>
Tested-by: default avatarDan Williams <dcbw@redhat.com>
Tested-by: default avatarMing Lei <tom.leiming@gmail.com>
Reviewed-by: default avatarOliver Neukum <oliver@neukum.org>
Acked-by: default avatarAlan Cox <alan@linux.intel.com>
Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@suse.de>
parent 09106974

drivers/usb/serial/usb-serial.c

+68 −36
Original line number Original line Diff line number Diff line
@@ -137,22 +137,10 @@ static void destroy_serial(struct kref *kref) 




	dbg("%s - %s", __func__, serial->type->description);

	dbg("%s - %s", __func__, serial->type->description);





	serial->type->shutdown(serial);



	/* return the minor range that this device had */

	/* return the minor range that this device had */

	if (serial->minor != SERIAL_TTY_NO_MINOR)

	if (serial->minor != SERIAL_TTY_NO_MINOR)

		return_serial(serial);

		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

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

	 * not get cleaned up in port_release() as it was never registered with

	 * the driver core */

	 * 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 *serial;

	struct usb_serial_port *port;

	struct usb_serial_port *port;

	unsigned int portNumber;

	unsigned int portNumber;

	int retval;

	int retval = 0;





	dbg("%s", __func__);

	dbg("%s", __func__);
@@ -198,21 +186,24 @@ static int serial_open (struct tty_struct *tty, struct file *filp) 
		return -ENODEV;

		return -ENODEV;

	}

	}





	mutex_lock(&serial->disc_mutex);

	portNumber = tty->index - serial->minor;

	portNumber = tty->index - serial->minor;

	port = serial->port[portNumber];

	port = serial->port[portNumber];

	if (!port) {

	if (!port || serial->disconnected)

		retval = -ENODEV;

		goto bailout_kref_put;

	}



	if (port->serial->disconnected) {

		retval = -ENODEV;

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

	if (mutex_lock_interruptible(&port->mutex)) {

		retval = -ERESTARTSYS;

		retval = -ERESTARTSYS;

		goto bailout_kref_put;

		goto bailout_port_put;

	}

	}





	++port->port.count;

	++port->port.count;
@@ -232,14 +223,20 @@ static int serial_open (struct tty_struct *tty, struct file *filp) 
			goto bailout_mutex_unlock;

			goto bailout_mutex_unlock;

		}

		}





		mutex_lock(&serial->disc_mutex);

		if (serial->disconnected)

			retval = -ENODEV;

		else

			retval = usb_autopm_get_interface(serial->interface);

			retval = usb_autopm_get_interface(serial->interface);

		if (retval)

		if (retval)

			goto bailout_module_put;

			goto bailout_module_put;



		/* only call the device specific open if this

		/* only call the device specific open if this

		 * is the first time the port is opened */

		 * is the first time the port is opened */

		retval = serial->type->open(tty, port, filp);

		retval = serial->type->open(tty, port, filp);

		if (retval)

		if (retval)

			goto bailout_interface_put;

			goto bailout_interface_put;

		mutex_unlock(&serial->disc_mutex);

	}

	}





	mutex_unlock(&port->mutex);

	mutex_unlock(&port->mutex);
@@ -248,13 +245,16 @@ static int serial_open (struct tty_struct *tty, struct file *filp) 
bailout_interface_put:

bailout_interface_put:

	usb_autopm_put_interface(serial->interface);

	usb_autopm_put_interface(serial->interface);

bailout_module_put:

bailout_module_put:

	mutex_unlock(&serial->disc_mutex);

	module_put(serial->type->driver.owner);

	module_put(serial->type->driver.owner);

bailout_mutex_unlock:

bailout_mutex_unlock:

	port->port.count = 0;

	port->port.count = 0;

	tty->driver_data = NULL;

	tty->driver_data = NULL;

	tty_port_tty_set(&port->port, NULL);

	tty_port_tty_set(&port->port, NULL);

	mutex_unlock(&port->mutex);

	mutex_unlock(&port->mutex);

bailout_kref_put:

bailout_port_put:

	put_device(&port->dev);

bailout_serial_put:

	usb_serial_put(serial);

	usb_serial_put(serial);

	return retval;

	return retval;

}

}
@@ -262,6 +262,9 @@ bailout_kref_put: 
static void serial_close(struct tty_struct *tty, struct file *filp)

static void serial_close(struct tty_struct *tty, struct file *filp)

{

{

	struct usb_serial_port *port = tty->driver_data;

	struct usb_serial_port *port = tty->driver_data;

	struct usb_serial *serial;

	struct module *owner;

	int count;





	if (!port)

	if (!port)

		return;

		return;
@@ -269,6 +272,8 @@ static void serial_close(struct tty_struct *tty, struct file *filp) 
	dbg("%s - port %d", __func__, port->number);

	dbg("%s - port %d", __func__, port->number);





	mutex_lock(&port->mutex);

	mutex_lock(&port->mutex);

	serial = port->serial;

	owner = serial->type->driver.owner;





	if (port->port.count == 0) {

	if (port->port.count == 0) {

		mutex_unlock(&port->mutex);

		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

		 * this before we drop the port count. The call is protected

		 * by the port mutex

		 * 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)) {

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

		struct tty_struct *tty = tty_port_tty_get(&port->port);

		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;

	--port->port.count;



	count = port->port.count;

	mutex_unlock(&port->mutex);

	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,

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)

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

	kill_traffic(port);

	cancel_work_sync(&port->work);



	usb_free_urb(port->read_urb);

	usb_free_urb(port->read_urb);

	usb_free_urb(port->write_urb);

	usb_free_urb(port->write_urb);

	usb_free_urb(port->interrupt_in_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->bulk_out_buffer);

	kfree(port->interrupt_in_buffer);

	kfree(port->interrupt_in_buffer);

	kfree(port->interrupt_out_buffer);

	kfree(port->interrupt_out_buffer);

	flush_scheduled_work();		/* port->work */

	kfree(port);

	kfree(port);

}

}
@@ -1043,6 +1059,12 @@ void usb_serial_disconnect(struct usb_interface *interface) 
	usb_set_intfdata(interface, NULL);

	usb_set_intfdata(interface, NULL);

	/* must set a flag, to signal subdrivers */

	/* must set a flag, to signal subdrivers */

	serial->disconnected = 1;

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

	for (i = 0; i < serial->num_ports; ++i) {

		port = serial->port[i];

		port = serial->port[i];

		if (port) {

		if (port) {
@@ -1052,11 +1074,21 @@ void usb_serial_disconnect(struct usb_interface *interface) 
				tty_kref_put(tty);

				tty_kref_put(tty);

			}

			}

			kill_traffic(port);

			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

	/* let the last holder of this object

	 * cause it to be cleaned up */

	 * cause it to be cleaned up */

	mutex_unlock(&serial->disc_mutex);

	usb_serial_put(serial);

	usb_serial_put(serial);

	dev_info(dev, "device disconnected\n");

	dev_info(dev, "device disconnected\n");

}

}

CRA Git | Maintained and supported by SUSTech CRA and CCSE