Re: USB Ooops PL2303 when unplug while use (linux v3.7.3)

[Johan Hovold <jhovold-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[ Remembered to drop Alan Cox from Cc. ]

On Wed, Feb 13, 2013 at 01:40:08PM -0500, Peter Hurley wrote:
> On Wed, 2013-02-13 at 15:25 +0100, Johan Hovold wrote:
> > On Fri, Feb 01, 2013 at 10:44:25AM +0800, Chris Ruehl wrote:
> > > I file a report for you, please have a look when you have time.
> > 
> > Thanks for the bug report, Chris.
> > 
> > You have come across what looks like a known issue, which since it's
> > discovery last summer has been made worse by an unrelated change.
> > 
> > A similar oops was reported and its cause identified in this thread:
> > 
> > 	http://marc.info/?l=linux-usb&m=133837337927749&w=2
> > 
> > It turns out that the tty-layer may call the driver's dtr_rts even after
> > the device has been disconnected and the tty device unregistered. Since
> > last summer another change has made the problem worse by setting the
> > port data to NULL which results in even more drivers hitting the
> > problem.
> 
> The tty driver's close() routine must be called even if the open()
> failed because the tty layer doesn't know if the driver left unfinished
> business and is expecting to receive a close() to cleanup even if it
> failed the open(). This behavior was just recently documented in
> include/linux/tty_driver.h (ie., is in linux-next).

It's correct that tty-driver close() is called from tty_release() as part
of a failed open (which I mentioned elsewhere), but the tty-port
implementation has never called the tty-port callback shutdown() on
ports that fail to open. [ This is implemented using ASYNC_INITIALIZED. ]

In usb-serial, shutdown() is implemented as a call to the serial-driver
close(), and the serial drivers are expected to clean up any failed
open in their error paths instead (as is any driver using tty ports).

We have a situation where we are asked to re-open a HUPPED port (before
it's been fully closed). The hang-up was initiated by usb-serial core
which has received the disconnect() call. We set a disconnected flag
before calling vhangup() and refuse any further open (activate())
attempts.

This prevent DTR/RTS from being raised and also shutdown() from being
called. So it does not seem to make any sense for the tty-port
implementation to try to lower DTR/RTS on this never opened (or
initialised) port. [ Nor to honour port drain delay, for that matter. ]

What I'm trying to achieve with my RFC-patches -- apart from fixing a
few related issues -- is to get the tty-port implementation to uphold
the invariant of never making tty-port callbacks on a port that has
never been initialised (i.e. activate() has failed) just like we do with
shutdown() today -- at least during the error path of tty open().

> > While waiting for input from the tty-guru Alan Cox, and as the immediate
> > cause of that oops was remedied (by moving the offending interface
> > access in the driver in question), the problem was unfortunately
> > forgotten (or rather down-prioritised) until now.
> 
> Looks to me like a bug the usb serial mini-port interface design.
> A usb bus disconnect has the pl2303 (and every other) mini-port freeing
> the private data (before unregistering with tty anyway -- not that
> putting that first would fix the problem).
> 
> static int usb_serial_device_remove(struct device *dev)
> {
> 	struct usb_serial_driver *driver;
> 	struct usb_serial_port *port;
> 	int retval = 0;
> 	int minor;
> 
> 	port = to_usb_serial_port(dev);
> 	if (!port)
> 		return -ENODEV;
> 
> 	/* make sure suspend/resume doesn't race against port_remove */
> 	usb_autopm_get_interface(port->serial->interface);
> 
> 	device_remove_file(&port->dev, &dev_attr_port_number);
> 
> 	driver = port->serial->type;
> 	if (driver->port_remove)
> ====>		retval = driver->port_remove(port);
>
> 	minor = port->number;
> 	tty_unregister_device(usb_serial_tty_driver, minor);
> 	dev_info(dev, "%s converter now disconnected from ttyUSB%d\n",
> 		 driver->description, minor);
> 
> 	usb_autopm_put_interface(port->serial->interface);
> 	return retval;
> }
> 
> The pl2303 mini-port dutifully:
> 
> static int pl2303_port_remove(struct usb_serial_port *port)
> {
> 	struct pl2303_private *priv;
> 
> 	priv = usb_get_serial_port_data(port);
> ===>	kfree(priv);
> 
> 	return 0;
> }
> 
> while the tty layer still has outstanding references to the port.
> 
> static void pl2303_dtr_rts(struct usb_serial_port *port, int on)
> {
> =====>	struct pl2303_private *priv = usb_get_serial_port_data(port);
> 	unsigned long flags;
> 	u8 control;
> 
> 	[...]
> 
> 
> The tty layer (and its port implementation) can't protect the pl2303
> mini-port from this behavior/interface design.
> 
> It's the glue layer's responsibility to correctly manage the differing
> lifetimes of its bus devices with tty devices (and tty_ports, if used).
> 
> Meaning, that if the physical device disconnects from the bus, the ports
> must simply become in-operative; they can't disappear.

Agreed, the usb-serial port implementation and disconnect handling
appears broken.

I'll see if I can fix this up.

This patch ("USB: serial: fix null-pointer dereferences on disconnect")
will take care of the dtr_rts callback on a disconnected port, either
way.

> BTW, just fixing this one part won't work because the usb serial driver
> is also abruptly deleting the usb_serial_port device as well:
> 
> static void usb_serial_disconnect(struct usb_interface *interface)
> {
> 	[...]
> 
> 	for (i = 0; i < serial->num_ports; ++i) {
> 		port = serial->port[i];
> 		if (port) {
> 			struct tty_struct *tty = tty_port_tty_get(&port->port);
> 			if (tty) {
> 				tty_vhangup(tty);
> 				tty_kref_put(tty);
> 			}
> 			kill_traffic(port);
> 			cancel_work_sync(&port->work);
> 			if (device_is_registered(&port->dev))
> ========>			device_del(&port->dev);
> 		}
> 	}
> 	
> 	[...]
> }
> 
> Ummm, wasn't that where the port private data pointer was?

Indeed.

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html