[patch]fix last races in usb serial

Hi,

this is John's patch for fixing the last disconnect/IO races in usb-serial.
I ported it to Greg's current tree.
To explain how it works, I quote him:

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

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]>
Signed-off-by: Oliver Neukum <[email protected]>

-- 

--- a/drivers/usb/serial/usb-serial.c	2007-05-03 11:12:01.000000000 +0200
+++ b/drivers/usb/serial/usb-serial.c	2007-05-03 12:26:19.000000000 +0200
@@ -61,9 +61,41 @@ static struct usb_driver usb_serial_driv
 
 static int debug;
 static struct usb_serial *serial_table[SERIAL_TTY_MINORS];	/* initially all NULL */
-static spinlock_t table_lock;
+static unsigned long ports_reserved[SERIAL_TTY_MINORS/BITS_PER_LONG];
+static spinlock_t table_lock; /* protects serial_table and ports_reserved */
 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.
+
+	ports_reserved is an array of bits parallel to usb_serial.
+	get_free_serial uses this array to locate a range of TTY devices, which are not yet registered.
+	Then get_free_serial reserves the found range by setting bits.
+	After that usb_serial_probe registers TTY devices in this range using device_register.
+	A bit is cleared by destroy_serial after it unregisters devices using device_unregister.
+*/
+
+void usb_serial_put(struct usb_serial *serial);
+
 struct usb_serial *usb_serial_get_by_index(unsigned index)
 {
 	struct usb_serial *serial;
@@ -92,7 +124,7 @@ static struct usb_serial *get_free_seria
 
 		good_spot = 1;
 		for (j = 1; j <= num_ports-1; ++j)
-			if ((i+j >= SERIAL_TTY_MINORS) || (serial_table[i+j])) {
+			if ((i+j >= SERIAL_TTY_MINORS) || (test_bit(i+j, ports_reserved))) {
 				good_spot = 0;
 				i += j;
 				break;
@@ -104,7 +136,9 @@ static struct usb_serial *get_free_seria
 		j = 0;
 		dbg("%s - minor base = %d", __FUNCTION__, *minor);
 		for (i = *minor; (i < (*minor + num_ports)) && (i < SERIAL_TTY_MINORS); ++i) {
+			kref_get(&serial->kref);
 			serial_table[i] = serial;
+			__set_bit(i, ports_reserved);
 			serial->port[j++]->number = i;
 		}
 		spin_unlock(&table_lock);
@@ -126,10 +160,82 @@ 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);
 }
 
+static int usb_serial_check_reserved(unsigned index)
+{
+	int ret;
+
+	spin_lock(&table_lock);
+	ret = test_bit(index, ports_reserved);
+	spin_unlock(&table_lock);
+	return ret;
+}
+
+static void usb_serial_unreserve(unsigned index, unsigned num_ports)
+{
+	int i;
+
+	spin_lock(&table_lock);
+	for (i = 0; i < num_ports; ++i, ++index) {
+		__clear_bit(index, ports_reserved);
+	}
+	spin_unlock(&table_lock);
+}
+
+/* 
+ * serial_lock expects a valid usb_serial * serial pointer.
+ * serial_lock returns 0 if usb_serial is still active and the caller can use it.
+ *	 In this case the caller must call serial_unlock.
+ * serial_lock returns -ENODEV 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 = -ENODEV;
+	} else {
+		serial->lock_count++;
+		ret = 0;
+	}	 
+	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;
+	spin_unlock_irqrestore(&serial->lock, flags);
+	wait_event(serial->shutdown_wait, serial->lock_count == 0);
+}
+
 static void destroy_serial(struct kref *kref)
 {
 	struct usb_serial *serial;
@@ -140,10 +246,8 @@ 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);
+	usb_serial_unreserve(serial->minor, serial->num_ports);
 
 	for (i = 0; i < serial->num_ports; ++i)
 		serial->port[i]->open_count = 0;
@@ -193,7 +297,11 @@ static int serial_open (struct tty_struc
 	/* get the serial object associated with this tty pointer */
 	serial = usb_serial_get_by_index(tty->index);
 	if (!serial) {
-		tty->driver_data = NULL;
+		/* There may be other files open for this port after device disconnection.
+			in this case don't erase tty->driver_data */
+		if (!usb_serial_check_reserved(tty->index)) {
+			tty->driver_data = NULL;
+		}
 		return -ENODEV;
 	}
 
@@ -204,9 +312,13 @@ static int serial_open (struct tty_struc
 		goto bailout_kref_put;
 	}
 
+	retval = serial_lock(serial);
+	if (retval)
+		goto bailout_kref_put;
+
 	if (mutex_lock_interruptible(&port->mutex)) {
 		retval = -ERESTARTSYS;
-		goto bailout_kref_put;
+		goto bailout_kref_put_unlock;
 	}
 	 
 	++port->open_count;
@@ -234,6 +346,7 @@ static int serial_open (struct tty_struc
 	}
 
 	mutex_unlock(&port->mutex);
+	serial_unlock(serial);
 	return 0;
 
 bailout_module_put:
@@ -243,6 +356,8 @@ bailout_mutex_unlock:
 	tty->driver_data = NULL;
 	port->tty = NULL;
 	mutex_unlock(&port->mutex);
+bailout_kref_put_unlock:
+	serial_unlock(serial);
 bailout_kref_put:
 	usb_serial_put(serial);
 	return retval;
@@ -299,8 +414,13 @@ static int serial_write (struct tty_stru
 		goto exit;
 	}
 
+	retval = serial_lock(port->serial); 
+	if (retval)
+		goto exit;
+
 	/* pass on to the driver specific version of this function */
 	retval = port->serial->type->write(port, buf, count);
+	serial_unlock(port->serial);
 
 exit:
 	return retval;
@@ -321,9 +441,12 @@ static int serial_write_room (struct tty
 		goto exit;
 	}
 
+	retval = serial_lock(port->serial); 
+	if (retval)
+		goto exit;
 	/* pass on to the driver specific version of this function */
 	retval = port->serial->type->write_room(port);
-
+	serial_unlock(port->serial);
 exit:
 	return retval;
 }
@@ -343,8 +466,12 @@ static int serial_chars_in_buffer (struc
 		goto exit;
 	}
 
+	retval = serial_lock(port->serial); 
+	if (retval)
+		goto exit;
 	/* pass on to the driver specific version of this function */
 	retval = port->serial->type->chars_in_buffer(port);
+	serial_unlock(port->serial);
 
 exit:
 	return retval;
@@ -364,9 +491,13 @@ static void serial_throttle (struct tty_
 		return;
 	}
 
+	if(serial_lock(port->serial))
+		return;
 	/* 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)
@@ -383,9 +514,13 @@ static void serial_unthrottle (struct tt
 		return;
 	}
 
+	if(serial_lock(port->serial))
+		return;
 	/* 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)
@@ -403,11 +538,16 @@ static int serial_ioctl (struct tty_stru
 		goto exit;
 	}
 
+	retval = serial_lock(port->serial); 
+	if (retval)
+		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;
+	serial_unlock(port->serial);
+
 
 exit:
 	return retval;
@@ -427,9 +567,13 @@ static void serial_set_termios (struct t
 		return;
 	}
 
+	if(serial_lock(port->serial))
+		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);
+	serial_unlock(port->serial);
+
 }
 
 static void serial_break (struct tty_struct *tty, int break_state)
@@ -446,9 +590,12 @@ static void serial_break (struct tty_str
 		return;
 	}
 
+	if(serial_lock(port->serial))
+		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);
+	serial_unlock(port->serial);
 }
 
 static int serial_read_proc (char *page, char **start, off_t off, int count, int *eof, void *data)
@@ -465,6 +612,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)
@@ -481,6 +630,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;
 		}
@@ -488,6 +638,7 @@ static int serial_read_proc (char *page,
 			begin += length;
 			length = 0;
 		}
+		serial_unlock(serial);
 		usb_serial_put(serial);
 	}
 	*eof = 1;
@@ -501,6 +652,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;
@@ -512,16 +664,23 @@ static int serial_tiocmget (struct tty_s
 		return -ENODEV;
 	}
 
+	retval = serial_lock(port->serial); 
+	if(retval) 
+		goto exit;
 	if (port->serial->type->tiocmget)
-		return port->serial->type->tiocmget(port, file);
-
-	return -EINVAL;
+		retval = port->serial->type->tiocmget(port, file);
+	else
+		retval = -EINVAL;
+	serial_unlock(port->serial);
+exit:
+	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;
@@ -533,10 +692,17 @@ static int serial_tiocmset (struct tty_s
 		return -ENODEV;
 	}
 
+	retval = serial_lock(port->serial); 
+	if (retval)
+		goto exit;
 	if (port->serial->type->tiocmset)
-		return port->serial->type->tiocmset(port, file, set, clear);
+		retval = port->serial->type->tiocmset(port, file, set, clear);
+	else
+		retval = -EINVAL;
+	serial_unlock(port->serial);
 
-	return -EINVAL;
+exit:
+	return retval;
 }
 
 /*
@@ -614,6 +780,9 @@ static struct usb_serial * create_serial
 	serial->interface = interface;
 	kref_init(&serial->kref);
 
+	spin_lock_init(&serial->lock);
+	init_waitqueue_head(&serial->shutdown_wait);
+
 	return serial;
 }
 
@@ -1057,6 +1226,9 @@ void usb_serial_disconnect(struct usb_in
 
 	usb_set_intfdata (interface, NULL);
 	if (serial) {
+		
+		return_serial(serial); /* erase pointers to serial from serial_table */
+
 		for (i = 0; i < serial->num_ports; ++i) {
 			port = serial->port[i];
 			if (port) {
@@ -1065,8 +1237,20 @@ 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");
--- a/include/linux/usb/serial.h	2007-03-23 15:51:38.610525100 -0800
+++ b/include/linux/usb/serial.h	2007-03-23 17:16:58.524274200 -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)

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel