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
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
