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

Am Dienstag, 20. März 2007 00:56 schrieb John:
> 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.

Hi John,

this was so long ago that I made a patch based on the discussion
we had back then. Would you mind having a look at it? We should
merge those patches to get an optimum.

	Regards
		Oliver

--- a/include/linux/usb/serial.h	2007-03-16 01:32:25.000000000 +0100
+++ b/include/linux/usb/serial.h	2007-03-16 20:32:05.000000000 +0100
@@ -128,6 +128,7 @@
 	struct usb_device *		dev;
 	struct usb_serial_driver *	type;
 	struct usb_interface *		interface;
+	spinlock_t			lock;
 	unsigned char			minor;
 	unsigned char			num_ports;
 	unsigned char			num_port_pointers;
@@ -135,6 +136,7 @@
 	char				num_interrupt_out;
 	char				num_bulk_in;
 	char				num_bulk_out;
+	int				busy;
 	struct usb_serial_port *	port[MAX_NUM_PORTS];
 	struct kref			kref;
 	void *				private;
--- a/drivers/usb/serial/usb-serial.c	2007-03-16 01:32:38.000000000 +0100
+++ b/drivers/usb/serial/usb-serial.c	2007-03-16 21:24:47.000000000 +0100
@@ -59,8 +59,10 @@
 
 static int debug;
 static struct usb_serial *serial_table[SERIAL_TTY_MINORS];	/* initially all NULL */
+static unsigned long deathrow[SERIAL_TTY_MINORS/BITS_PER_LONG];			/* disconnected but open*/
 static spinlock_t table_lock;
 static LIST_HEAD(usb_serial_driver_list);
+static DECLARE_WAIT_QUEUE_HEAD(usbserial_henchman);
 
 struct usb_serial *usb_serial_get_by_index(unsigned index)
 {
@@ -104,6 +106,7 @@
 		for (i = *minor; (i < (*minor + num_ports)) && (i < SERIAL_TTY_MINORS); ++i) {
 			serial_table[i] = serial;
 			serial->port[j++]->number = i;
+			__clear_bit(i, deathrow);
 		}
 		spin_unlock(&table_lock);
 		return serial;
@@ -112,6 +115,53 @@
 	return NULL;
 }
 
+/* requires locking from caller*/
+static int on_deathrow(struct usb_serial_port *port)
+{
+	int number = port->number;
+
+	return test_bit(number, deathrow); 
+}
+
+/* requires locking from caller*/
+static void schedule_execution(struct usb_serial_port *port)
+{
+	int number = port->number;
+
+	__set_bit(number, deathrow);
+}
+
+static int lock_serial(struct usb_serial_port *port)
+{
+	struct usb_serial *serial = port->serial;
+	unsigned long flags;
+	int terminated;
+
+	spin_lock_irqsave(&serial->lock, flags);
+	terminated = on_deathrow(port);
+	if (!terminated)
+		serial->busy++;
+	spin_unlock_irqrestore(&serial->lock, flags);
+
+	if (terminated)
+		return -ENODEV;
+	return 0;
+}
+
+static void unlock_serial(struct usb_serial_port *port)
+{
+	struct usb_serial *serial = port->serial;
+	unsigned long flags;
+	int waiters;
+
+	spin_lock_irqsave(&serial->lock, flags);
+	waiters = --serial->busy;
+	spin_unlock_irqrestore(&serial->lock, flags);
+
+	if (!waiters)
+		wake_up(&usbserial_henchman);
+}
+
 static void return_serial(struct usb_serial *serial)
 {
 	int i;
@@ -148,9 +198,6 @@
 			serial->port[i] = NULL;
 		}
 
-	if (serial->type->shutdown)
-		serial->type->shutdown(serial);
-
 	/* return the minor range that this device had */
 	return_serial(serial);
 
@@ -203,9 +250,13 @@
 		goto bailout_kref_put;
 	}
 
+	retval = lock_serial(port);
+	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;
@@ -233,6 +284,7 @@
 	}
 
 	mutex_unlock(&port->mutex);
+	unlock_serial(port);
 	return 0;
 
 bailout_module_put:
@@ -242,6 +294,8 @@
 	tty->driver_data = NULL;
 	port->tty = NULL;
 	mutex_unlock(&port->mutex);
+bailout_kref_put_unlock:
+	unlock_serial(port);
 bailout_kref_put:
 	usb_serial_put(serial);
 	return retval;
@@ -298,9 +352,15 @@
 		goto exit;
 	}
 
+	retval = lock_serial(port);
+	if (retval)
+		goto exit;
+
 	/* pass on to the driver specific version of this function */
 	retval = port->serial->type->write(port, buf, count);
 
+	unlock_serial(port);
+
 exit:
 	return retval;
 }
@@ -320,9 +380,14 @@
 		goto exit;
 	}
 
+	retval = lock_serial(port);
+	if (retval)
+		goto exit;
 	/* pass on to the driver specific version of this function */
 	retval = port->serial->type->write_room(port);
 
+	unlock_serial(port);
+
 exit:
 	return retval;
 }
@@ -342,9 +407,13 @@
 		goto exit;
 	}
 
+	retval = lock_serial(port);
+	if (retval)
+		goto exit;
 	/* pass on to the driver specific version of this function */
 	retval = port->serial->type->chars_in_buffer(port);
 
+	unlock_serial(port);
 exit:
 	return retval;
 }
@@ -364,8 +433,12 @@
 	}
 
 	/* pass on to the driver specific version of this function */
-	if (port->serial->type->throttle)
+	if (port->serial->type->throttle) {
+		if (0 < lock_serial(port))
+			return;
 		port->serial->type->throttle(port);
+		unlock_serial(port);
+	}
 }
 
 static void serial_unthrottle (struct tty_struct * tty)
@@ -383,8 +456,12 @@
 	}
 
 	/* pass on to the driver specific version of this function */
-	if (port->serial->type->unthrottle)
+	if (port->serial->type->unthrottle) {
+		if (0 < lock_serial(port))
+			return;
 		port->serial->type->unthrottle(port);
+		unlock_serial(port);
+	}
 }
 
 static int serial_ioctl (struct tty_struct *tty, struct file * file, unsigned int cmd, unsigned long arg)
@@ -402,12 +479,16 @@
 		goto exit;
 	}
 
+	retval = lock_serial(port);
+	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;
-
+	
+	unlock_serial(port);
 exit:
 	return retval;
 }
@@ -427,8 +508,12 @@
 	}
 
 	/* pass on to the driver specific version of this function if it is available */
-	if (port->serial->type->set_termios)
+	if (port->serial->type->set_termios) {
+		if (0 < lock_serial(port))
+			return;
 		port->serial->type->set_termios(port, old);
+		unlock_serial(port);
+	}
 }
 
 static void serial_break (struct tty_struct *tty, int break_state)
@@ -446,8 +531,12 @@
 	}
 
 	/* pass on to the driver specific version of this function if it is available */
-	if (port->serial->type->break_ctl)
+	if (port->serial->type->break_ctl) {
+		if (0 < lock_serial(port))
+			return;
 		port->serial->type->break_ctl(port, break_state);
+		unlock_serial(port);
+	}
 }
 
 static int serial_read_proc (char *page, char **start, off_t off, int count, int *eof, void *data)
@@ -500,6 +589,7 @@
 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;
@@ -511,16 +601,26 @@
 		return -ENODEV;
 	}
 
+	retval = lock_serial(port);
+	if (retval)
+		goto exit;
+
 	if (port->serial->type->tiocmget)
-		return port->serial->type->tiocmget(port, file);
+		retval = port->serial->type->tiocmget(port, file);
+	else
+		retval = -EINVAL;
+
+	unlock_serial(port);
+exit:
 
-	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;
@@ -532,10 +632,18 @@
 		return -ENODEV;
 	}
 
+	retval = lock_serial(port);
+	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;
 
-	return -EINVAL;
+	unlock_serial(port);
+exit:
+	return retval;
 }
 
 /*
@@ -612,6 +720,7 @@
 	serial->type = driver;
 	serial->interface = interface;
 	kref_init(&serial->kref);
+	spin_lock_init(&serial->lock);
 
 	return serial;
 }
@@ -1059,11 +1168,19 @@
 		for (i = 0; i < serial->num_ports; ++i) {
 			port = serial->port[i];
 			if (port) {
+				spin_lock_irq(&serial->lock);
+				schedule_execution(port);
+				spin_unlock_irq(&serial->lock);
 				if (port->tty)
 					tty_hangup(port->tty);
 				kill_traffic(port);
 			}
 		}
+		wait_event(usbserial_henchman, serial->busy == 0);
+		/* allow the subdriver to cleanly kill
+		 * private URBs */
+		if (serial->type->shutdown)
+			(serial->type->shutdown)(serial);
 		/* let the last holder of this object 
 		 * cause it to be cleaned up */
 		usb_serial_put(serial);

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