Re: [PATCH] usb: fix possible oops in serial option driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


On Fri, May 25, 2012 at 01:46:48PM -0400, Alan Stern wrote:
> On Fri, 25 May 2012, Johan Hovold wrote:
> 
> > > Well tty-core shouldn't handle disconnect (which is usb-serial-
> > > specific)
> > 
> > Just to clarify: the tty layer could learn to deal with disconnect
> > (useful also for cdc-acm for example), but currently it is handled in
> > usb-serial core so moving it there first for dtr_rts makes sense.
> 
> The tty layer may not know how to handle disconnect, but it certainly 
> does know about handling tty_unregister_device().  Since that routine 
> gets called by the usb-serial core as part of disconnect processing, 
> what more is really needed?

You're right, and this is indeed a bug in the tty layer. In fact, there
are three different issues interacting here which are related to the
oops Jan discovered:

	1. hangup race in tty
	2. access to interface after disconnect
	3. io after disconnect (e.g. during close)


1. hangup race in tty
---------------------
First the call to lower dtr/rts should never happen after hangup --
tty_close_start checks tty_hung_up_p(filp) and returns if hung up.

I'm able to reproduce the race (and oops) reliably here using the pl2303
driver. The bug is triggered when an open occurs after vhangup has been
called from disconnect. That open fails since the device has been
disconnected (-ENODEV from serial_install), causing tty_open to call
tty_release which ends up proceeding with port close despite having a
received a hangup and still having ports open (normally dtr/rts is only
lowered on the last close). Here a log from such a run:

[ t0	     ] tty_open - filp = f50c2d80

	open the device once, then open and close in a loop and finally
	disconnect the device

  ...

[  293.705143] tty_open - filp = f5100a80
[  293.705173] tty_open: opening ttyUSB0...
[  293.705186] pl2303_dtr_rts - on = 1
[  293.705249] ttyUSB0 vhangup...

	hangup due to disconnect

[  293.705254] tty_release: ttyUSB0 (tty count=2)...
[  293.705259] tty_port_close
[  293.705297] __tty_hangup - setting hung_up_tty_fops, filp = f50c2d80
[  293.705302] tty_open - filp = f5100f00

	open after hangup -- get's the normal tty_fops...

[  293.705423] tty_open: opening ttyUSB0...
[  293.705441] tty_open: error -19 in opening ttyUSB0...
[  293.705470] tty_release: ttyUSB0 (tty count=2)...

	open fails -- calls release which calls close

[  293.705478] tty_port_close
[  293.705489] tty_port_close_start - closing, filp = f5100f00
[  293.705499] tty_port_close_start - drain_delay

	close proceeds despite hangup --
	schedule_timeout_interruptible() because drain delay is set for
	pl2303

[  293.705795] pl2303 ttyUSB0: pl2303 converter now disconnected from ttyUSB0
[  293.705865] usbserial_generic 6-1:1.0: device disconnected

	usb_serial_disconnect returns

[  293.971228] tty_port_close_start - dropping dtr_rts
[  293.971254] pl2303_dtr_rts - on = 0

	dropping dtr/rts after hangup, disconnect, and
	tty_device_unregister (and with open ports)

[  293.971860] tty_release: ttyUSB0 (tty count=1)...
[  293.971935] tty_port_close
[  293.971946] tty_port_close_start - hung up

	last port (opened at t0) closes (without touching dtr/rts)

[  293.971963] tty_release: freeing tty structure...


2. access to interface after disconnect
---------------------------------------
The oops occurs when dereferencing serial->interface->cur_altsetting. In
my setup even the interface condition has a strange value, e.g.:

[ 1005.517927] pl2303_dtr_rts - serial = f53b8540, intf = f53f3000, cur_alt = 00766564
[ 1005.517938] pl2303_dtr_rts - condition = 559, unreg = 1

Documentation clearly states that we cannot use the interface after
returning from disconnect. In fact there's no reason for option to be
messing around with the interface in send_setup. I'm preparing a patch
which determines whether to use send_setup (check for blacklisting) and
store the interface number at probe instead.

This will make the oops go away, even with the tty race still there.


3. io after disconnect (e.g. during close)
------------------------------------------
You asked in reply to my patch why dtr_rts was special and needed to be
protected against disconnect. It's treated with extra care by several
drivers since Oliver introduced the disconnected flag:

	http://www.mail-archive.com/linux-usb@xxxxxxxxxxxxxxx/msg00207.html

It used to be handled in driver close() but was later factored out to to
dtr_rts(). My patch simply refactored this protection to usb-serial
core.

But as far as I can see, with the modifications that has since been done
to the tty layer, if the tty layer handled hangup in cases such as the
above we could get rid of this special handling as well?

In fact, it seems to me we can even get rid of the disconnected checks
in close as vhangup will either call driver close (shutdown) or not
return until an ongoing close has finished. This should be enough to
uphold the rule: "You are not allowed any IO to a device after returning
from this [disconnect] callback" which was what the disconnected checks
in close was introduced for, right?

Johan
--
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


B and H Foto and Electronics Corp.

[Linux Media]     [Video for Linux]     [Linux Input]     [Linux Audio Users]     [Photo]     [Yosemite News]    [Yosemite Photos]    [Free Online Dating]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]     [More Archives]

Add to Google Powered by Linux