On Wed, May 30, 2012 at 10:00:14AM +0200, Bjørn Mork wrote:
> We need to make sure that the USB serial driver we find
> matches the USB driver whose probe we are currently
> executing. Otherwise we will end up with USB serial
> devices bound to the correct serial driver but wrong
> USB driver.
>
> An example of such cross-probing, where the usbserial_generic
> USB driver has found the sierra serial driver:
>
> May 29 18:26:15 nemi kernel: [ 4442.559246] usbserial_generic 4-4:1.0: Sierra USB modem converter detected
> May 29 18:26:20 nemi kernel: [ 4447.556747] usbserial_generic 4-4:1.2: Sierra USB modem converter detected
> May 29 18:26:25 nemi kernel: [ 4452.557288] usbserial_generic 4-4:1.3: Sierra USB modem converter detected
>
> sysfs view of the same problem:
>
> bjorn@nemi:~$ ls -l /sys/bus/usb/drivers/sierra/
> total 0
> --w------- 1 root root 4096 May 29 18:23 bind
> lrwxrwxrwx 1 root root 0 May 29 18:23 module -> ../../../../module/usbserial
> --w------- 1 root root 4096 May 29 18:23 uevent
> --w------- 1 root root 4096 May 29 18:23 unbind
> bjorn@nemi:~$ ls -l /sys/bus/usb-serial/drivers/sierra/
> total 0
> --w------- 1 root root 4096 May 29 18:23 bind
> lrwxrwxrwx 1 root root 0 May 29 18:23 module -> ../../../../module/sierra
> -rw-r--r-- 1 root root 4096 May 29 18:23 new_id
> lrwxrwxrwx 1 root root 0 May 29 18:32 ttyUSB0 -> ../../../../devices/pci0000:00/0000:00:1d.7/usb4/4-4/4-4:1.0/ttyUSB0
> lrwxrwxrwx 1 root root 0 May 29 18:32 ttyUSB1 -> ../../../../devices/pci0000:00/0000:00:1d.7/usb4/4-4/4-4:1.2/ttyUSB1
> lrwxrwxrwx 1 root root 0 May 29 18:32 ttyUSB2 -> ../../../../devices/pci0000:00/0000:00:1d.7/usb4/4-4/4-4:1.3/ttyUSB2
> --w------- 1 root root 4096 May 29 18:23 uevent
> --w------- 1 root root 4096 May 29 18:23 unbind
>
> bjorn@nemi:~$ ls -l /sys/bus/usb/drivers/usbserial_generic/
> total 0
> lrwxrwxrwx 1 root root 0 May 29 18:33 4-4:1.0 -> ../../../../devices/pci0000:00/0000:00:1d.7/usb4/4-4/4-4:1.0
> lrwxrwxrwx 1 root root 0 May 29 18:33 4-4:1.2 -> ../../../../devices/pci0000:00/0000:00:1d.7/usb4/4-4/4-4:1.2
> lrwxrwxrwx 1 root root 0 May 29 18:33 4-4:1.3 -> ../../../../devices/pci0000:00/0000:00:1d.7/usb4/4-4/4-4:1.3
> --w------- 1 root root 4096 May 29 18:33 bind
> lrwxrwxrwx 1 root root 0 May 29 18:33 module -> ../../../../module/usbserial
> --w------- 1 root root 4096 May 29 18:22 uevent
> --w------- 1 root root 4096 May 29 18:33 unbind
> bjorn@nemi:~$ ls -l /sys/bus/usb-serial/drivers/generic/
> total 0
> --w------- 1 root root 4096 May 29 18:33 bind
> lrwxrwxrwx 1 root root 0 May 29 18:33 module -> ../../../../module/usbserial
> -rw-r--r-- 1 root root 4096 May 29 18:33 new_id
> --w------- 1 root root 4096 May 29 18:22 uevent
> --w------- 1 root root 4096 May 29 18:33 unbind
>
> So we end up with a mismatch between the USB driver and the
> USB serial driver. The reason for the above is simple: The
> USB driver probe will succeed if *any* registered serial
> driver matches, and will use that serial driver for all
> serial driver functions.
>
> This makes ref counting go wrong. We count the USB driver
> as used, but not the USB serial driver. This may result
> in Oops'es as demonstrated by Johan Hovold <jhovold@xxxxxxxxx>:
>
> [11811.646396] drivers/usb/serial/usb-serial.c: get_free_serial 1
> [11811.646443] drivers/usb/serial/usb-serial.c: get_free_serial - minor base = 0
> [11811.646460] drivers/usb/serial/usb-serial.c: usb_serial_probe - registering ttyUSB0
> [11811.646766] usb 6-1: pl2303 converter now attached to ttyUSB0
> [11812.264197] USB Serial deregistering driver FTDI USB Serial Device
> [11812.264865] usbcore: deregistering interface driver ftdi_sio
> [11812.282180] USB Serial deregistering driver pl2303
> [11812.283141] pl2303 ttyUSB0: pl2303 converter now disconnected from ttyUSB0
> [11812.283272] usbcore: deregistering interface driver pl2303
> [11812.301056] USB Serial deregistering driver generic
> [11812.301186] usbcore: deregistering interface driver usbserial_generic
> [11812.301259] drivers/usb/serial/usb-serial.c: usb_serial_disconnect
> [11812.301823] BUG: unable to handle kernel paging request at f8e7438c
> [11812.301845] IP: [<f8e38445>] usb_serial_disconnect+0xb5/0x100 [usbserial]
> [11812.301871] *pde = 357ef067 *pte = 00000000
> [11812.301957] Oops: 0000 [#1] PREEMPT SMP
> [11812.301983] Modules linked in: usbserial(-) [last unloaded: pl2303]
> [11812.302008]
> [11812.302019] Pid: 1323, comm: modprobe Tainted: G W 3.4.0-rc7+ #101 Dell Inc. Vostro 1520/0T816J
> [11812.302115] EIP: 0060:[<f8e38445>] EFLAGS: 00010246 CPU: 1
> [11812.302130] EIP is at usb_serial_disconnect+0xb5/0x100 [usbserial]
> [11812.302141] EAX: f508a180 EBX: f508a180 ECX: 00000000 EDX: f8e74300
> [11812.302151] ESI: f5050800 EDI: 00000001 EBP: f5141e78 ESP: f5141e58
> [11812.302160] DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
> [11812.302170] CR0: 8005003b CR2: f8e7438c CR3: 34848000 CR4: 000007d0
> [11812.302180] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> [11812.302189] DR6: ffff0ff0 DR7: 00000400
> [11812.302199] Process modprobe (pid: 1323, ti=f5140000 task=f61e2bc0 task.ti=f5140000)
> [11812.302209] Stack:
> [11812.302216] f8e3be0f f8e3b29c f8e3ae00 00000000 f513641c f5136400 f513641c f507a540
> [11812.302325] f5141e98 c133d2c1 00000000 00000000 f509c400 f513641c f507a590 f5136450
> [11812.302372] f5141ea8 c12f0344 f513641c f507a590 f5141ebc c12f0c67 00000000 f507a590
> [11812.302419] Call Trace:
> [11812.302439] [<c133d2c1>] usb_unbind_interface+0x51/0x190
> [11812.302456] [<c12f0344>] __device_release_driver+0x64/0xb0
> [11812.302469] [<c12f0c67>] driver_detach+0x97/0xa0
> [11812.302483] [<c12f001c>] bus_remove_driver+0x6c/0xe0
> [11812.302500] [<c145938d>] ? __mutex_unlock_slowpath+0xcd/0x140
> [11812.302514] [<c12f0ff9>] driver_unregister+0x49/0x80
> [11812.302528] [<c1457df6>] ? printk+0x1d/0x1f
> [11812.302540] [<c133c50d>] usb_deregister+0x5d/0xb0
> [11812.302557] [<f8e37c55>] ? usb_serial_deregister+0x45/0x50 [usbserial]
> [11812.302575] [<f8e37c8d>] usb_serial_deregister_drivers+0x2d/0x40 [usbserial]
> [11812.302593] [<f8e3a6e2>] usb_serial_generic_deregister+0x12/0x20 [usbserial]
> [11812.302611] [<f8e3acf0>] usb_serial_exit+0x8/0x32 [usbserial]
> [11812.302716] [<c1080b48>] sys_delete_module+0x158/0x260
> [11812.302730] [<c110594e>] ? mntput+0x1e/0x30
> [11812.302746] [<c145c3c3>] ? sysenter_exit+0xf/0x18
> [11812.302746] [<c107777c>] ? trace_hardirqs_on_caller+0xec/0x170
> [11812.302746] [<c145c390>] sysenter_do_call+0x12/0x36
> [11812.302746] Code: 24 02 00 00 e8 dd f3 20 c8 f6 86 74 02 00 00 02 74 b4 8d 86 4c 02 00 00 47 e8 78 55 4b c8 0f b6 43 0e 39 f8 7f a9 8b 53 04 89 d8 <ff> 92 8c 00 00 00 89 d8 e8 0e ff ff ff 8b 45 f0 c7 44 24 04 2f
> [11812.302746] EIP: [<f8e38445>] usb_serial_disconnect+0xb5/0x100 [usbserial] SS:ESP 0068:f5141e58
> [11812.302746] CR2: 00000000f8e7438c
>
> Fix by only evaluating serial drivers pointing back to the
> USB driver we are currently probing. This still allows two
> or more drivers to match the same device, running their
> serial driver probes to sort out which one to use.
>
> Cc: stable@xxxxxxxxxxxxxxx # 3.0, 3.2, 3.3, 3.4
> Cc: Johan Hovold <jhovold@xxxxxxxxx>
> Signed-off-by: Bjørn Mork <bjorn@xxxxxxx>
This looks fine to my eyes, FWIW:
Reviewed-by: Felipe Balbi <balbi@xxxxxx>
> ---
> > So search_serial_device needs to do the match. Does anyone see a
> > problem with that? I guess noone here does, unless I express it in code
> > :-) So I'll go do that now.
>
> Here it is. Changes in
> v2:
> - moved driver matching to search_serial_device to allow two drivers
> to match the same device
> - much more verbose patch description including relevant parts of my
> previous comment as suggested by Felipe Balbi
> - including example Oops from Johan Hovold in description
>
> This problem became evident when the generic serial driver probe was
> removed, but has been in the code "forever". I believe it can be
> triggered on older kernels by using new_id on serial drivers trusting
> device matching. I have verified that the fix applies unmodified to
> stable/linux-3.0.y
> stable/linux-3.2.y
> stable/linux-3.3.y
> stable/linux-3.4.y
> and I believe it should go into all of them. I have not verified the
> end result on these kernels, but the affected logic has not changed so
> it should work.
>
>
> drivers/usb/serial/usb-serial.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
> index 6a1b609..6e8c527 100644
> --- a/drivers/usb/serial/usb-serial.c
> +++ b/drivers/usb/serial/usb-serial.c
> @@ -659,12 +659,14 @@ exit:
> static struct usb_serial_driver *search_serial_device(
> struct usb_interface *iface)
> {
> - const struct usb_device_id *id;
> + const struct usb_device_id *id = NULL;
> struct usb_serial_driver *drv;
> + struct usb_driver *driver = to_usb_driver(iface->dev.driver);
>
> /* Check if the usb id matches a known device */
> list_for_each_entry(drv, &usb_serial_driver_list, driver_list) {
> - id = get_iface_id(drv, iface);
> + if (drv->usb_driver == driver)
> + id = get_iface_id(drv, iface);
> if (id)
> return drv;
> }
> --
> 1.7.10
>
--
balbi
Attachment:
signature.asc
Description: Digital signature
