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
