[PATCH 1/1] race condition fixes for usb-serial

This is a fix for race condition in usb-serial.c 
between serial_open and usb_serial_disconnect
(kernel version 2.6.21-rc3)
Prior to this patch instances of usb_serial were not
ref-counted consistently, which caused serial_open
to reuse already deleted instance.
For more details search Web for 
"Possible race condition in usb-serial.c" 
discussion in Dec 2006.

The present patch ensures that instances
of usb_serial are ref-counted consistently
by calling kref_get in get_free_serial 
and usb_serial_put in return_serial.

Another problem was that the code prior to
this patch called serial->type->shutdown(serial)
from destroy_serial, which may execute after 
usb_serial_disconnect (if the serial device was opened).
This was an error because no IO should be done
on a USB device after usb_serial_disconnect returns.

To fix that the present patch moved call to
serial->type->shutdown(serial)
from destroy_serial to usb_serial_disconnect.
Also it introduces functions
serial_lock, serial_unlock and 
serial_lock_and_wait_before_shutdown.
serial_lock and serial_unlock are called 
from each serial_* routine and 
serial_lock_and_wait_before_shutdown
is called from usb_serial_disconnect
to ensure that all serial_* calls exit before 
usb_serial_disconnect calls 
serial->type->shutdown(serial),

Signed-off-by: John Green <[email protected]>
---
diff -uprN a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
--- a/drivers/usb/serial/usb-serial.c	2007-03-15 15:56:56.848654500 -0800
+++ b/drivers/usb/serial/usb-serial.c	2007-03-19 11:43:13.236067600 -0800
@@ -62,6 +62,31 @@ static struct usb_serial *serial_table[S
 static spinlock_t table_lock;
 static LIST_HEAD(usb_serial_driver_list);
 
+/*
+	usb_serial objects are ref counted. This is necessary because pointers to an object
+	may be stored in several places. Each time a pointer is duplicated, the ref count
+	should be incremented. This done using kref_get(&serial->kref). When pointer is
+	destroyed, the count should be decremented using usb_serial_put(serial).
+	A typical life cycle of usb_serial:
+
+	1. usb_serial_probe creates usb_serial (ref count set to 1) and stores the pointer 
+	   in the usb_interface using usb_set_intfdata.
+	2. usb_serial_probe calls get_free_serial, which copies the pointer one or more times 
+	   to serial_table indexed by TTY minor index (ref count is incremeted for each copy).
+	3. serial_open takes this pointer from serial_table (ref count incremented) and passes it to TTY 
+	   (this is done indirectly: tty->driver_data = port, but port->serial = serial).
+	4. serial_close decrements the ref count (because tty object is destroyed).
+	5. usb_serial_disconnect calls return_serial, which erases all pointers to the given serial
+	   from serial_table (decrementing the ref count).
+	6. usb_serial_disconnect destroys the pointer, which was stored in usb_interface
+	   (decrementing the ref count). 
+	At this point the ref count normally reaches 0 and the object destroyed. 
+	Alternatively, serial_close may be called after usb_serial_disconnect and the object destroyed
+	at that time.
+*/
+
+void usb_serial_put(struct usb_serial *serial);
+
 struct usb_serial *usb_serial_get_by_index(unsigned index)
 {
 	struct usb_serial *serial;
@@ -100,8 +125,10 @@ static struct usb_serial *get_free_seria
 
 		*minor = i;
 		dbg("%s - minor base = %d", __FUNCTION__, *minor);
-		for (i = *minor; (i < (*minor + num_ports)) && (i < SERIAL_TTY_MINORS); ++i)
+		for (i = *minor; (i < (*minor + num_ports)) && (i < SERIAL_TTY_MINORS); ++i) {
+			kref_get(&serial->kref);
 			serial_table[i] = serial;
+		}
 		spin_unlock(&table_lock);
 		return serial;
 	}
@@ -121,10 +148,65 @@ static void return_serial(struct usb_ser
 	spin_lock(&table_lock);
 	for (i = 0; i < serial->num_ports; ++i) {
 		serial_table[serial->minor + i] = NULL;
+		usb_serial_put(serial);
 	}
 	spin_unlock(&table_lock);
 }
 
+/* 
+ * serial_lock expects a valid usb_serial * serial pointer.
+ * serial_lock returns 1 if usb_serial is still active and the caller can use it.
+ *	 In this case the caller must call serial_unlock.
+ * serial_lock returns 0 if disconnection routine was already called on the device. 
+ *	 In this case the caller should not perform any IO operations, but should return an error code.
+ */
+int serial_lock(struct usb_serial * serial)
+{
+	int ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&serial->lock, flags);
+	if (serial->shutdown_called) {
+		ret = 0;
+	} else {
+		serial->lock_count++;
+		ret = 1;
+	}	 
+	spin_unlock_irqrestore(&serial->lock, flags);
+	return ret;
+}
+
+void serial_unlock(struct usb_serial * serial)
+{
+	int wakeup = 0;
+	unsigned long flags;
+
+	spin_lock_irqsave(&serial->lock, flags);
+	serial->lock_count--;
+	if (serial->shutdown_called && serial->lock_count == 0) {
+		wakeup = 1;
+	}
+	spin_unlock_irqrestore(&serial->lock, flags);
+
+	if(wakeup) {
+		wake_up(&serial->shutdown_wait);
+	}
+}
+
+void serial_lock_and_wait_before_shutdown(struct usb_serial * serial)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&serial->lock, flags);
+	serial->shutdown_called = 1;
+	while(serial->lock_count > 0) {
+		spin_unlock_irqrestore(&serial->lock, flags);
+		wait_event(serial->shutdown_wait, serial->lock_count == 0);
+		spin_lock_irqsave(&serial->lock, flags);
+	}
+	spin_unlock_irqrestore(&serial->lock, flags);
+}
+
 static void destroy_serial(struct kref *kref)
 {
 	struct usb_serial *serial;
@@ -135,11 +217,6 @@ static void destroy_serial(struct kref *
 
 	dbg("%s - %s", __FUNCTION__, serial->type->description);
 
-	serial->type->shutdown(serial);
-
-	/* return the minor range that this device had */
-	return_serial(serial);
-
 	for (i = 0; i < serial->num_ports; ++i)
 		serial->port[i]->open_count = 0;
 
@@ -294,8 +371,13 @@ static int serial_write (struct tty_stru
 		goto exit;
 	}
 
-	/* pass on to the driver specific version of this function */
-	retval = port->serial->type->write(port, buf, count);
+	if(serial_lock(port->serial)) {
+		/* pass on to the driver specific version of this function */
+		retval = port->serial->type->write(port, buf, count);
+		serial_unlock(port->serial);
+	} else {
+		retval = -ENODEV;
+	}
 
 exit:
 	return retval;
@@ -316,9 +398,13 @@ static int serial_write_room (struct tty
 		goto exit;
 	}
 
-	/* pass on to the driver specific version of this function */
-	retval = port->serial->type->write_room(port);
-
+	if(serial_lock(port->serial)) {
+		/* pass on to the driver specific version of this function */
+		retval = port->serial->type->write_room(port);
+		serial_unlock(port->serial);
+	} else {
+		retval = -ENODEV;
+	}
 exit:
 	return retval;
 }
@@ -338,8 +424,13 @@ static int serial_chars_in_buffer (struc
 		goto exit;
 	}
 
-	/* pass on to the driver specific version of this function */
-	retval = port->serial->type->chars_in_buffer(port);
+	if(serial_lock(port->serial)) {
+		/* pass on to the driver specific version of this function */
+		retval = port->serial->type->chars_in_buffer(port);
+		serial_unlock(port->serial);
+	} else {
+		retval = -ENODEV;
+	}
 
 exit:
 	return retval;
@@ -359,9 +450,13 @@ static void serial_throttle (struct tty_
 		return;
 	}
 
-	/* pass on to the driver specific version of this function */
-	if (port->serial->type->throttle)
-		port->serial->type->throttle(port);
+	if(serial_lock(port->serial)) {
+		/* pass on to the driver specific version of this function */
+		if (port->serial->type->throttle)
+			port->serial->type->throttle(port);
+		serial_unlock(port->serial);
+	}
+
 }
 
 static void serial_unthrottle (struct tty_struct * tty)
@@ -378,9 +473,13 @@ static void serial_unthrottle (struct tt
 		return;
 	}
 
-	/* pass on to the driver specific version of this function */
-	if (port->serial->type->unthrottle)
-		port->serial->type->unthrottle(port);
+	if(serial_lock(port->serial)) {
+		/* pass on to the driver specific version of this function */
+		if (port->serial->type->unthrottle)
+			port->serial->type->unthrottle(port);
+		serial_unlock(port->serial);
+	}
+
 }
 
 static int serial_ioctl (struct tty_struct *tty, struct file * file, unsigned int cmd, unsigned long arg)
@@ -398,11 +497,17 @@ static int serial_ioctl (struct tty_stru
 		goto exit;
 	}
 
-	/* pass on to the driver specific version of this function if it is available */
-	if (port->serial->type->ioctl)
-		retval = port->serial->type->ioctl(port, file, cmd, arg);
-	else
-		retval = -ENOIOCTLCMD;
+	if(serial_lock(port->serial)) {
+		/* pass on to the driver specific version of this function if it is available */
+		if (port->serial->type->ioctl)
+			retval = port->serial->type->ioctl(port, file, cmd, arg);
+		else
+			retval = -ENOIOCTLCMD;
+		serial_unlock(port->serial);
+	} else {
+		retval = -ENODEV;
+	}
+
 
 exit:
 	return retval;
@@ -422,9 +527,13 @@ static void serial_set_termios (struct t
 		return;
 	}
 
-	/* pass on to the driver specific version of this function if it is available */
-	if (port->serial->type->set_termios)
-		port->serial->type->set_termios(port, old);
+	if(serial_lock(port->serial)) {
+		/* pass on to the driver specific version of this function if it is available */
+		if (port->serial->type->set_termios)
+			port->serial->type->set_termios(port, old);
+		serial_unlock(port->serial);
+	}
+
 }
 
 static void serial_break (struct tty_struct *tty, int break_state)
@@ -441,9 +550,12 @@ static void serial_break (struct tty_str
 		return;
 	}
 
-	/* pass on to the driver specific version of this function if it is available */
-	if (port->serial->type->break_ctl)
-		port->serial->type->break_ctl(port, break_state);
+	if(serial_lock(port->serial)) {
+		/* pass on to the driver specific version of this function if it is available */
+		if (port->serial->type->break_ctl)
+			port->serial->type->break_ctl(port, break_state);
+		serial_unlock(port->serial);
+	}
 }
 
 static int serial_read_proc (char *page, char **start, off_t off, int count, int *eof, void *data)
@@ -460,6 +572,8 @@ static int serial_read_proc (char *page,
 		serial = usb_serial_get_by_index(i);
 		if (serial == NULL)
 			continue;
+		if(!serial_lock(serial))
+			continue;
 
 		length += sprintf (page+length, "%d:", i);
 		if (serial->type->driver.owner)
@@ -476,6 +590,7 @@ static int serial_read_proc (char *page,
 			
 		length += sprintf (page+length, "\n");
 		if ((length + begin) > (off + count)) {
+			serial_unlock(serial);
 			usb_serial_put(serial);
 			goto done;
 		}
@@ -483,6 +598,7 @@ static int serial_read_proc (char *page,
 			begin += length;
 			length = 0;
 		}
+		serial_unlock(serial);
 		usb_serial_put(serial);
 	}
 	*eof = 1;
@@ -496,6 +612,7 @@ done:
 static int serial_tiocmget (struct tty_struct *tty, struct file *file)
 {
 	struct usb_serial_port *port = tty->driver_data;
+	int retval;
 
 	if (!port)
 		return -ENODEV;
@@ -507,16 +624,24 @@ static int serial_tiocmget (struct tty_s
 		return -ENODEV;
 	}
 
-	if (port->serial->type->tiocmget)
-		return port->serial->type->tiocmget(port, file);
+	if(serial_lock(port->serial)) {
+		if (port->serial->type->tiocmget)
+			retval = port->serial->type->tiocmget(port, file);
+		else
+			retval = -EINVAL;
+		serial_unlock(port->serial);
+	} else {
+		retval = -ENODEV;
+	}
 
-	return -EINVAL;
+	return retval;
 }
 
 static int serial_tiocmset (struct tty_struct *tty, struct file *file,
 			    unsigned int set, unsigned int clear)
 {
 	struct usb_serial_port *port = tty->driver_data;
+	int retval;
 
 	if (!port)
 		return -ENODEV;
@@ -528,10 +653,17 @@ static int serial_tiocmset (struct tty_s
 		return -ENODEV;
 	}
 
-	if (port->serial->type->tiocmset)
-		return port->serial->type->tiocmset(port, file, set, clear);
+	if(serial_lock(port->serial)) {
+		if (port->serial->type->tiocmset)
+			retval = port->serial->type->tiocmset(port, file, set, clear);
+		else
+			retval = -EINVAL;
+		serial_unlock(port->serial);
+	} else {
+		retval = -ENODEV;
+	}
 
-	return -EINVAL;
+	return retval;
 }
 
 /*
@@ -609,6 +741,11 @@ static struct usb_serial * create_serial
 	serial->interface = interface;
 	kref_init(&serial->kref);
 
+	spin_lock_init(&serial->lock);
+	serial->lock_count = 0;
+	serial->shutdown_called = 0;
+	init_waitqueue_head(&serial->shutdown_wait);
+
 	return serial;
 }
 
@@ -1056,6 +1193,12 @@ void usb_serial_disconnect(struct usb_in
 
 	usb_set_intfdata (interface, NULL);
 	if (serial) {
+		
+		/* return the minor range that this device had.
+		 * after that all future serial_open calls on this device will fail.
+		 */
+		return_serial(serial);
+
 		for (i = 0; i < serial->num_ports; ++i) {
 			port = serial->port[i];
 			if (port) {
@@ -1064,8 +1207,21 @@ void usb_serial_disconnect(struct usb_in
 				kill_traffic(port);
 			}
 		}
-		/* let the last holder of this object 
-		 * cause it to be cleaned up */
+
+		/* The following call will block if any serial_* routine (on our serial) 
+		 * is still in progress. It will wait for all such routines to exit.
+		 * After that all further calls to serial_* routines will return immediately
+		 * without performing any IO or touching the content of serial or port.
+		 * Therefore, we can cleanup serial and ports.
+		 */
+		serial_lock_and_wait_before_shutdown(serial);
+
+		serial->type->shutdown(serial);
+
+		/* Now we destroy the pointer to serial, which was associated with usb_interface.
+		 * This is likely to delete serial object, unless some TTY files are still open.
+		 * In that case the object will be deleted from serial_close.
+		 */
 		usb_serial_put(serial);
 	}
 	dev_info(dev, "device disconnected\n");
diff -uprN a/include/linux/usb/serial.h b/include/linux/usb/serial.h
--- a/include/linux/usb/serial.h	2007-03-15 15:57:22.472834500 -0800
+++ b/include/linux/usb/serial.h	2007-03-15 18:30:28.821108400 -0800
@@ -119,6 +119,11 @@ static inline void usb_set_serial_port_d
  * @num_bulk_in: number of bulk in endpoints we have
  * @num_bulk_out: number of bulk out endpoints we have
  * @port: array of struct usb_serial_port structures for the different ports.
+ * @kref: refcount for storing the number of pointers to usb_serial
+ * @lock: spinlock to protect access to lock_count and shutdown_called
+ * @lock_count: number of serial_* routines, which locked usb_serial and are using it.
+ * @shutdown_called: If this flag is set, serial_* routines must not use other fields of usb_serial.
+ * @shutdown_wait: used by usb_serial_disconnect to block until all serial_* routines exit.
  * @private: place to put any driver specific information that is needed.  The
  *	usb-serial driver is required to manage this data, the usb-serial core
  *	will not touch this.  Use usb_get_serial_data() and
@@ -136,7 +141,11 @@ struct usb_serial {
 	char				num_bulk_in;
 	char				num_bulk_out;
 	struct usb_serial_port *	port[MAX_NUM_PORTS];
-	struct kref			kref;
+	struct kref 			kref;
+	spinlock_t			lock;
+	int 				lock_count;
+	int 				shutdown_called;
+	wait_queue_head_t		shutdown_wait;
 	void *				private;
 };
 #define to_usb_serial(d) container_of(d, struct usb_serial, kref)


-------------------------------------------------------

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel