On Mon, 2009-04-06 at 17:08 -0400, Alan Stern wrote: > On Tue, 24 Mar 2009, Dan Williams wrote: > > > > Dan, you can try this one out in place of the earlier patch. Hopefully > > > it will resolve the use-after-free BUG. > > > > This patch (slightly modified to apply to 2.6.29-rc8 sources; the > > bailout_serial_put stuff didn't apply exactly) appears to work correctly > > over a few plug/unplug sessions while PPP is up and running on the tty. > > Modified patch below. > > Dan: > > I've got a revised version of the patch that needs testing. Will you > be so kind as to oblige? This is against 2.6.29. Yes, this appears to work correctly. I do get BUG (sleeping in invalid context) [1] right after insertion. Doesn't seem related to me, but who knows? Thanks! Dan [1] http://www.kerneloops.org/submitresult.php?number=341383 > Thanks, > > Alan Stern > > > Index: 2.6.29/drivers/usb/serial/usb-serial.c > =================================================================== > --- 2.6.29.orig/drivers/usb/serial/usb-serial.c > +++ 2.6.29/drivers/usb/serial/usb-serial.c > @@ -142,16 +142,6 @@ static void destroy_serial(struct kref * > if (serial->minor != SERIAL_TTY_NO_MINOR) > return_serial(serial); > > - for (i = 0; i < serial->num_ports; ++i) > - serial->port[i]->port.count = 0; > - > - /* the ports are cleaned up and released in port_release() */ > - for (i = 0; i < serial->num_ports; ++i) > - if (serial->port[i]->dev.parent != NULL) { > - device_unregister(&serial->port[i]->dev); > - serial->port[i] = NULL; > - } > - > /* If this is a "fake" port, we have to clean it up here, as it will > * not get cleaned up in port_release() as it was never registered with > * the driver core */ > @@ -186,7 +176,7 @@ static int serial_open (struct tty_struc > struct usb_serial *serial; > struct usb_serial_port *port; > unsigned int portNumber; > - int retval; > + int retval = 0; > > dbg("%s", __func__); > > @@ -197,16 +187,24 @@ static int serial_open (struct tty_struc > return -ENODEV; > } > > + mutex_lock(&serial->disc_mutex); > portNumber = tty->index - serial->minor; > port = serial->port[portNumber]; > - if (!port) { > + if (!port || serial->disconnected) > retval = -ENODEV; > - goto bailout_kref_put; > - } > + else > + get_device(&port->dev); > + /* > + * Note: Our locking order requirement does not allow port->mutex > + * to be acquired while serial->disc_mutex is held. > + */ > + mutex_unlock(&serial->disc_mutex); > + if (retval) > + goto bailout_serial_put; > > if (mutex_lock_interruptible(&port->mutex)) { > retval = -ERESTARTSYS; > - goto bailout_kref_put; > + goto bailout_port_put; > } > > ++port->port.count; > @@ -226,9 +224,15 @@ static int serial_open (struct tty_struc > goto bailout_mutex_unlock; > } > > - retval = usb_autopm_get_interface(serial->interface); > + mutex_lock(&serial->disc_mutex); > + if (serial->disconnected) > + retval = -ENODEV; > + else > + retval = usb_autopm_get_interface(serial->interface); > + mutex_unlock(&serial->disc_mutex); > if (retval) > goto bailout_module_put; > + > /* only call the device specific open if this > * is the first time the port is opened */ > retval = serial->type->open(tty, port, filp); > @@ -248,7 +252,9 @@ bailout_mutex_unlock: > tty->driver_data = NULL; > tty_port_tty_set(&port->port, NULL); > mutex_unlock(&port->mutex); > -bailout_kref_put: > +bailout_port_put: > + put_device(&port->dev); > +bailout_serial_put: > usb_serial_put(serial); > return retval; > } > @@ -256,6 +262,9 @@ bailout_kref_put: > static void serial_close(struct tty_struct *tty, struct file *filp) > { > struct usb_serial_port *port = tty->driver_data; > + struct usb_serial *serial; > + struct module *owner; > + int count; > > if (!port) > return; > @@ -263,6 +272,8 @@ static void serial_close(struct tty_stru > dbg("%s - port %d", __func__, port->number); > > mutex_lock(&port->mutex); > + serial = port->serial; > + owner = serial->type->driver.owner; > > if (port->port.count == 0) { > mutex_unlock(&port->mutex); > @@ -275,7 +286,7 @@ static void serial_close(struct tty_stru > * this before we drop the port count. The call is protected > * by the port mutex > */ > - port->serial->type->close(tty, port, filp); > + serial->type->close(tty, port, filp); > > if (port->port.count == (port->console ? 2 : 1)) { > struct tty_struct *tty = tty_port_tty_get(&port->port); > @@ -289,17 +300,23 @@ static void serial_close(struct tty_stru > } > } > > - if (port->port.count == 1) { > - mutex_lock(&port->serial->disc_mutex); > - if (!port->serial->disconnected) > - usb_autopm_put_interface(port->serial->interface); > - mutex_unlock(&port->serial->disc_mutex); > - module_put(port->serial->type->driver.owner); > - } > --port->port.count; > - > + count = port->port.count; > mutex_unlock(&port->mutex); > - usb_serial_put(port->serial); > + put_device(&port->dev); > + > + /* Mustn't dereference port any more */ > + if (count == 0) { > + mutex_lock(&serial->disc_mutex); > + if (!serial->disconnected) > + usb_autopm_put_interface(serial->interface); > + mutex_unlock(&serial->disc_mutex); > + } > + usb_serial_put(serial); > + > + /* Mustn't dereference serial any more */ > + if (count == 0) > + module_put(owner); > } > > static int serial_write(struct tty_struct *tty, const unsigned char *buf, > @@ -548,7 +565,13 @@ static void kill_traffic(struct usb_seri > > static void port_free(struct usb_serial_port *port) > { > + /* > + * Stop all the traffic before cancelling the work, so that > + * nobody will restart it by calling usb_serial_port_softint. > + */ > kill_traffic(port); > + cancel_work_sync(&port->work); > + > usb_free_urb(port->read_urb); > usb_free_urb(port->write_urb); > usb_free_urb(port->interrupt_in_urb); > @@ -557,7 +580,6 @@ static void port_free(struct usb_serial_ > kfree(port->bulk_out_buffer); > kfree(port->interrupt_in_buffer); > kfree(port->interrupt_out_buffer); > - flush_scheduled_work(); /* port->work */ > kfree(port); > } > > @@ -1042,6 +1064,8 @@ void usb_serial_disconnect(struct usb_in > usb_set_intfdata(interface, NULL); > /* must set a flag, to signal subdrivers */ > serial->disconnected = 1; > + mutex_unlock(&serial->disc_mutex); > + > for (i = 0; i < serial->num_ports; ++i) { > port = serial->port[i]; > if (port) { > @@ -1051,11 +1075,13 @@ void usb_serial_disconnect(struct usb_in > tty_kref_put(tty); > } > kill_traffic(port); > + cancel_work_sync(&port->work); > + device_unregister(&port->dev); > + serial->port[i] = NULL; > } > } > /* let the last holder of this object > * cause it to be cleaned up */ > - mutex_unlock(&serial->disc_mutex); > usb_serial_put(serial); > dev_info(dev, "device disconnected\n"); > } > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
