Hi, Sorry, I don't see how table_lock will fix the original problem, which I reported (race between serial_open and usb_serial_disconnect). I copied my original sequence of events below and I don't think that the proposed table_lock will change anything.
As I wrote before, the error is that usb-serial does not use ref counting consistently because it does not count every outstanding pointer. Specifically, it does not count pointers in serial_table. This means that when usb_serial_disconnect calls usb_serial_put, ref count will go to 0 and object deleted. At the same time (but before call to return_serial) serial_open will call usb_serial_get_by_index and increment the ref count from 0 to 1 (which should never happen if ref counting is implemented correctly). Please, tell me is I am wrong. Another think I wanted to clarify is whether it is safe to call serial->type->shutdown after usb_serial_disconnect already returned. You wrote that it is OK to call usb_kill_urb count, but it is not OK to do any IO. So, if we continue to call serial->type->shutdown after disconnect, how can we ensure that client serial usb drivers do not call any IO? Are there any documented rules for what clients are supposed to call and when? I feel that it would be beneficial to ensure that shutdown is always called within disconnect to simplify task of client driver writers. This, however, will require diconnect to wait on a semaphore to ensure that all serial_* operations finished. What do you think? Thank you John This is the copy of my original race sequence: A:->usb_serial_disconnect A: -> usb_serial_put (serial); A: -> kref_put A: if ((atomic_read(&kref->refcount) == 1) Suppose refcount is 1 A: -> release -> destroy_serial B: -> serial_open B: -> usb_serial_get_by_index B: serial = serial_table[index] B: -> kref_get(&serial->kref); A: -> return_serial(serial); A: serial_table[serial->minor + i] = NULL; A: -> kfree (serial); B: continue to use serial, which was already freed. --- Oliver Neukum <[EMAIL PROTECTED]> wrote: > Hi, > > this fixes: > > 1) serial table is not protected by locks. > kref_get() may be called on freed memory > 2) probe() exposes uninitialized devices > 3) disconnect() does not make sure there's no more > IO to unbound devices > > Please test. It's against 2.6.20-rc3 but should > apply widely. > > Regards > Oliver > > --- a/drivers/usb/serial/usb-serial.c 2007-01-01 > 01:53:20.000000000 +0100 > +++ b/drivers/usb/serial/usb-serial.c 2007-01-02 > 10:44:08.000000000 +0100 > @@ -59,14 +59,19 @@ > > static int debug; > static struct usb_serial > *serial_table[SERIAL_TTY_MINORS]; /* initially all > NULL */ > +static spinlock_t table_lock; > static LIST_HEAD(usb_serial_driver_list); > > struct usb_serial *usb_serial_get_by_index(unsigned > index) > { > - struct usb_serial *serial = serial_table[index]; > + struct usb_serial *serial; > + > + spin_lock(&table_lock); > + serial = serial_table[index]; > > if (serial) > kref_get(&serial->kref); > + spin_unlock(&table_lock); > return serial; > } > > @@ -78,6 +83,7 @@ > dbg("%s %d", __FUNCTION__, num_ports); > > *minor = 0; > + spin_lock(&table_lock); > for (i = 0; i < SERIAL_TTY_MINORS; ++i) { > if (serial_table[i]) > continue; > @@ -96,8 +102,10 @@ > dbg("%s - minor base = %d", __FUNCTION__, > *minor); > for (i = *minor; (i < (*minor + num_ports)) && (i > < SERIAL_TTY_MINORS); ++i) > serial_table[i] = serial; > + spin_unlock(&table_lock); > return serial; > } > + spin_unlock(&table_lock); > return NULL; > } > > @@ -110,9 +118,11 @@ > if (serial == NULL) > return; > > + spin_lock(&serial_table); > for (i = 0; i < serial->num_ports; ++i) { > serial_table[serial->minor + i] = NULL; > } > + spin_unlock(&serial_table); > } > > static void destroy_serial(struct kref *kref) > @@ -559,15 +569,20 @@ > port_free(port); > } > > -static void port_free(struct usb_serial_port *port) > +static void kill_traffic(struct usb_serial_port > *port) > { > usb_kill_urb(port->read_urb); > - usb_free_urb(port->read_urb); > usb_kill_urb(port->write_urb); > - usb_free_urb(port->write_urb); > usb_kill_urb(port->interrupt_in_urb); > - usb_free_urb(port->interrupt_in_urb); > usb_kill_urb(port->interrupt_out_urb); > +} > + > +static void port_free(struct usb_serial_port *port) > +{ > + kill_traffic(port); > + usb_free_urb(port->read_urb); > + usb_free_urb(port->write_urb); > + usb_free_urb(port->interrupt_in_urb); > usb_free_urb(port->interrupt_out_urb); > kfree(port->bulk_in_buffer); > kfree(port->bulk_out_buffer); > @@ -771,12 +786,6 @@ > num_ports = type->num_ports; > } > > - if (get_free_serial (serial, num_ports, &minor) == > NULL) { > - dev_err(&interface->dev, "No more free serial > devices\n"); > - kfree (serial); > - return -ENOMEM; > - } > - > serial->minor = minor; > serial->num_ports = num_ports; > serial->num_bulk_in = num_bulk_in; > @@ -925,6 +934,11 @@ > } > } > > + if (get_free_serial (serial, num_ports, &minor) == > NULL) { > + dev_err(&interface->dev, "No more free serial > devices\n"); > + goto probe_error; > + } > + > /* register all of the individual ports with the > driver core */ > for (i = 0; i < num_ports; ++i) { > port = serial->port[i]; > @@ -1002,8 +1016,11 @@ > if (serial) { > for (i = 0; i < serial->num_ports; ++i) { > port = serial->port[i]; > - if (port && port->tty) > - tty_hangup(port->tty); > + if (port) { > + if (port->tty) > + tty_hangup(port->tty); > + kill_traffic(port); > + } > } > /* let the last holder of this object > * cause it to be cleaned up */ > @@ -1040,6 +1057,7 @@ > return -ENOMEM; > > /* Initialize our global data */ > + spin_lock_init(&table_lock); > for (i = 0; i < SERIAL_TTY_MINORS; ++i) { > serial_table[i] = NULL; > } > __________________________________________________ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com ------------------------------------------------------------------------- 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