[BK PATCH] USB fixes for 2.6.7-rc3

[BK PATCH] USB fixes for 2.6.7-rc3

[Greg KH]

Hi,

Here are three bugfixes for the USB code against 2.6.7-rc3.  They do the
following:
	- fix deadlock in usbfs
	- fix bug and enable the pwc USB driver to build
	- fix bugs in cyberjack driver to enable device to work.

The first one is the most serious, but the others are good things to
have fixed in the final 2.6.7 release.

Please pull from:
	bk://kernel.bkbits.net/gregkh/linux/usb-fix-2.6

Patches will be posted to linux-usb-devel as a follow-up thread for
those who want to see them.

thanks,

greg k-h

 drivers/usb/core/devio.c       |    2 +-
 drivers/usb/media/Kconfig      |    2 +-
 drivers/usb/media/pwc-if.c     |    9 +--------
 drivers/usb/serial/cyberjack.c |   21 ++++++---------------
 include/linux/usb.h            |    1 +
 5 files changed, 10 insertions(+), 25 deletions(-)
-----

<kaie:kuix.de>:
  o USB: enable pwc usb camera driver

<siegfried.hildebrand:fernuni-hagen.de>:
  o USB: Fix problems with cyberjack usb-serial-module since kernel 2.6.2

Duncan Sands:
  o USB devio.c: deadlock fix

Oops w/ USB serial + modular ipaq

[Zwane Mwaikambo]

Loading the ipaq module, connecting a device and then unloading ipaq.ko
oopses.

PocketPC PDA ttyUSB0: PocketPC PDA converter now disconnected from ttyUSB0
usbserial 1-2:1.0: device disconnected
Unable to handle kernel NULL pointer dereference at virtual address 00000085
 printing eip:
f8990a6f
*pde = 00000000
Oops: 0000 [#1]
SMP
Modules linked in: ppp_async ppp_generic slhc ipaq usbserial vmnet vmmon
dm_mod e100 e1000 3c59x
CPU:    0
EIP:    0060:[<f8990a6f>]    Tainted: P   VLI
EFLAGS: 00210246   (2.6.7-rc3-mm2-slock)
EIP is at usb_serial_disconnect+0x1b/0x86 [usbserial]
eax: 00000000   ebx: 00000011   ecx: 00000002   edx: 00000000
esi: 00000000   edi: 00000001   ebp: d9d0c000   esp: d9d0cf38
ds: 007b   es: 007b   ss: 0068
Process rmmod (pid: 19148, threadinfo=d9d0c000 task=e62ced10)
Stack: f8993f40 c02ef8f6 c1d61b48 c1ec46d8 00000000 f8998220 f8990e04 f89924c0
       f8991e8c f8996cc8 f8998480 c04631a4 00000000 c0128b94 00000000 71617069
       00000000 d5d33a80 d5d33a80 c013cac9 40001000 40000000 c013ce17 40000000
Call Trace:
 [<c02ef8f6>] device_release_driver+0x56/0x58
 [<f8990e04>] usb_serial_deregister+0x9b/0x9f [usbserial]
 [<c0128b94>] sys_delete_module+0x132/0x180
 [<c013cac9>] unmap_vma_list+0xe/0x17
 [<c013ce17>] do_munmap+0x10a/0x144
 [<c0110df4>] do_page_fault+0x0/0x501
 [<c0103a71>] sysenter_past_esp+0x52/0x71

Code: 04 24 b3 1d 99 f8 e8 89 65 78 c7 e9 8a f5 ff ff 83 ec 18 89 5c 24 0c
89 7c 24 14 89 74 24 10 8d 58 10 89 c7 a1 04 46 99 f8 85 c0 <8b> 73 74 75
48 85 f6 c7 43 74 00 00 00 00 74 08 8d 46 38 e8 47

(gdb) list * usb_serial_disconnect+0x1b
0x1a6f is in usb_serial_disconnect (device.h:298).
293     }
294
295     static inline void *
296     dev_get_drvdata (struct device *dev)
297     {
298             return dev->driver_data;
299     }


The problem is due to the following;

void usb_serial_deregister(struct usb_serial_device_type *device)
{
	struct usb_serial *serial;
	int i;

	for(i = 0; i < SERIAL_TTY_MINORS; ++i) {
		serial = serial_table[i];
		if ((serial != NULL) && (serial->type == device)) {
			printk("usb_serial_deregister: %p %p\n", serial, serial->interface);
			usb_driver_release_interface (&usb_serial_driver, serial->interface);
			usb_serial_disconnect (serial->interface); <===
		}
	}
...
}

It's not safe to use serial->interface after that
usb_driver_release_interface(). The following patch works as a workaround
but i don't trust it as there may be a leak.

Index: linux-2.6.7/drivers/usb/serial/usb-serial.c
===================================================================
RCS file: /home/cvsroot/linux-2.6.7/drivers/usb/serial/usb-serial.c,v
retrieving revision 1.1.1.1
diff -u -p -B -r1.1.1.1 usb-serial.c
--- linux-2.6.7/drivers/usb/serial/usb-serial.c	16 Jun 2004 16:49:38 -0000	1.1.1.1
+++ linux-2.6.7/drivers/usb/serial/usb-serial.c	23 Jun 2004 06:29:56 -0000
@@ -1393,7 +1393,6 @@ void usb_serial_deregister(struct usb_se
 		serial = serial_table[i];
 		if ((serial != NULL) && (serial->type == device)) {
 			usb_driver_release_interface (&usb_serial_driver, serial->interface);
-			usb_serial_disconnect (serial->interface);
 		}
 	}

Re: Oops w/ USB serial + modular ipaq

[Zwane Mwaikambo]

On Wed, 23 Jun 2004, Zwane Mwaikambo wrote:

> Loading the ipaq module, connecting a device and then unloading ipaq.ko
> oopses.
>
> It's not safe to use serial->interface after that
> usb_driver_release_interface(). The following patch works as a workaround
> but i don't trust it as there may be a leak.
>
> Index: linux-2.6.7/drivers/usb/serial/usb-serial.c
> ===================================================================
> RCS file: /home/cvsroot/linux-2.6.7/drivers/usb/serial/usb-serial.c,v
> retrieving revision 1.1.1.1
> diff -u -p -B -r1.1.1.1 usb-serial.c
> --- linux-2.6.7/drivers/usb/serial/usb-serial.c	16 Jun 2004 16:49:38 -0000	1.1.1.1
> +++ linux-2.6.7/drivers/usb/serial/usb-serial.c	23 Jun 2004 06:29:56 -0000
> @@ -1393,7 +1393,6 @@ void usb_serial_deregister(struct usb_se
>  		serial = serial_table[i];
>  		if ((serial != NULL) && (serial->type == device)) {
>  			usb_driver_release_interface (&usb_serial_driver, serial->interface);
> -			usb_serial_disconnect (serial->interface);
>  		}
>  	}
>

I'll leave this patch going in a load/unload loop overnight and check on
slab.

Thanks,
	Zwane

Re: Oops w/ USB serial + modular ipaq

[Greg KH]

On Wed, Jun 23, 2004 at 02:33:25AM -0400, Zwane Mwaikambo wrote:
> Loading the ipaq module, connecting a device and then unloading ipaq.ko
> oopses.
> 
> PocketPC PDA ttyUSB0: PocketPC PDA converter now disconnected from ttyUSB0
> usbserial 1-2:1.0: device disconnected
> Unable to handle kernel NULL pointer dereference at virtual address 00000085
>  printing eip:
> f8990a6f
> *pde = 00000000
> Oops: 0000 [#1]
> SMP
> Modules linked in: ppp_async ppp_generic slhc ipaq usbserial vmnet vmmon
> dm_mod e100 e1000 3c59x
> CPU:    0
> EIP:    0060:[<f8990a6f>]    Tainted: P   VLI
> EFLAGS: 00210246   (2.6.7-rc3-mm2-slock)
> EIP is at usb_serial_disconnect+0x1b/0x86 [usbserial]
> eax: 00000000   ebx: 00000011   ecx: 00000002   edx: 00000000
> esi: 00000000   edi: 00000001   ebp: d9d0c000   esp: d9d0cf38
> ds: 007b   es: 007b   ss: 0068
> Process rmmod (pid: 19148, threadinfo=d9d0c000 task=e62ced10)
> Stack: f8993f40 c02ef8f6 c1d61b48 c1ec46d8 00000000 f8998220 f8990e04 f89924c0
>        f8991e8c f8996cc8 f8998480 c04631a4 00000000 c0128b94 00000000 71617069
>        00000000 d5d33a80 d5d33a80 c013cac9 40001000 40000000 c013ce17 40000000
> Call Trace:
>  [<c02ef8f6>] device_release_driver+0x56/0x58
>  [<f8990e04>] usb_serial_deregister+0x9b/0x9f [usbserial]
>  [<c0128b94>] sys_delete_module+0x132/0x180
>  [<c013cac9>] unmap_vma_list+0xe/0x17
>  [<c013ce17>] do_munmap+0x10a/0x144
>  [<c0110df4>] do_page_fault+0x0/0x501
>  [<c0103a71>] sysenter_past_esp+0x52/0x71
> 
> Code: 04 24 b3 1d 99 f8 e8 89 65 78 c7 e9 8a f5 ff ff 83 ec 18 89 5c 24 0c
> 89 7c 24 14 89 74 24 10 8d 58 10 89 c7 a1 04 46 99 f8 85 c0 <8b> 73 74 75
> 48 85 f6 c7 43 74 00 00 00 00 74 08 8d 46 38 e8 47
> 
> (gdb) list * usb_serial_disconnect+0x1b
> 0x1a6f is in usb_serial_disconnect (device.h:298).
> 293     }
> 294
> 295     static inline void *
> 296     dev_get_drvdata (struct device *dev)
> 297     {
> 298             return dev->driver_data;
> 299     }
> 
> 
> The problem is due to the following;
> 
> void usb_serial_deregister(struct usb_serial_device_type *device)
> {
> 	struct usb_serial *serial;
> 	int i;
> 
> 	for(i = 0; i < SERIAL_TTY_MINORS; ++i) {
> 		serial = serial_table[i];
> 		if ((serial != NULL) && (serial->type == device)) {
> 			printk("usb_serial_deregister: %p %p\n", serial, serial->interface);
> 			usb_driver_release_interface (&usb_serial_driver, serial->interface);
> 			usb_serial_disconnect (serial->interface); <===
> 		}
> 	}
> ...
> }
> 
> It's not safe to use serial->interface after that
> usb_driver_release_interface().

Why not?  The ->interface pointer is still valid, only thing is that
device does not have a driver bound to it anymore, so the later call to
dev_info() in the usb_serial_disconnect() call might cause the oops.

How about just switching those two calls around (usb_serial_disconnect()
before calling usb_driver_release_interface())?  Will that solve the
problem for you?  If you just comment out usb_serial_disconnect() you
will leak memory :(

thanks,

greg k-h

Re: Oops w/ USB serial + modular ipaq

[Zwane Mwaikambo]

On Wed, 23 Jun 2004, Greg KH wrote:

> > It's not safe to use serial->interface after that
> > usb_driver_release_interface().
>
> Why not?  The ->interface pointer is still valid, only thing is that
> device does not have a driver bound to it anymore, so the later call to
> dev_info() in the usb_serial_disconnect() call might cause the oops.

void usb_driver_release_interface(struct usb_driver *driver,
					struct usb_interface *iface)
{
	struct device *dev = &iface->dev;

...
	dev->driver = NULL;
	usb_set_intfdata(iface, NULL);
}

usb_set_intfdata() sets usb_interface->dev.driver_data to NULL, whilst in
usbserial_disconnect() we try and retrieve and use it;

void usb_serial_disconnect(struct usb_interface *interface)
{
	struct usb_serial *serial = usb_get_intfdata (interface);
	struct device *dev = &interface->dev;
...
}

> How about just switching those two calls around (usb_serial_disconnect()
> before calling usb_driver_release_interface())?  Will that solve the
> problem for you?

I tried it last night, switching around will cause an oops in
usb_driver_release_interface()

Unable to handle kernel paging request at virtual address 6b6b6beb
 printing eip:
c042da9a
*pde = 00000000
Oops: 0000 [#1]
PREEMPT SMP DEBUG_PAGEALLOC
Modules linked in: ipaq usbserial
CPU:    1
EIP:    0060:[<c042da9a>]    Not tainted
EFLAGS: 00010246   (2.6.7)
EIP is at usb_driver_release_interface+0xa/0x50
eax: 6b6b6b6b   ebx: 6b6b6b7b   ecx: 6b6b6b6b   edx: de0a5000
esi: 00000000   edi: e0ddda60   ebp: de0a5f34   esp: de0a5f30
ds: 007b   es: 007b   ss: 0068
Process rmmod (pid: 1414, threadinfo=de0a5000 task=d5c85a60)
Stack: dee0bec0 de0a5f54 e0e1b11c e0e1f1c0 6b6b6b6b d72baef8 e0dddcc0 c05dd3a8
       00000000 de0a5f64 e0ddbea7 e0ddda60 e0ddd9c0 de0a5fbc c0139c9e 00000000
       71617069 40000000 de0a5fa0 c0156f3d d647cdc8 de801a30 d647cdfc 40001000
Call Trace:
 [<c0107675>] show_stack+0x75/0x90
 [<c01077d5>] show_registers+0x125/0x180
 [<c010796b>] die+0xab/0x170
 [<c01185d8>] do_page_fault+0x1e8/0x535
 [<c01072cd>] error_code+0x2d/0x40
 [<e0e1b11c>] usb_serial_deregister+0x7c/0x90 [usbserial]
 [<e0ddbea7>] ipaq_exit+0x17/0x1b [ipaq]
 [<c0139c9e>] sys_delete_module+0x12e/0x190
 [<c0106139>] sysenter_past_esp+0x52/0x79

(gdb) list *usb_driver_release_interface+0xa
0x31a is in usb_driver_release_interface (drivers/usb/core/usb.c:347).
342                                             struct usb_interface *iface)
343     {
344             struct device *dev = &iface->dev;
345
346             /* this should never happen, don't release something that's not ours */
347             if (!dev->driver || dev->driver != &driver->driver)
348                     return;
349
350             /* don't disconnect from disconnect(), or before dev_add() */
351             if (!list_empty (&dev->driver_list) && !list_empty (&dev->bus_list))

0x00000310 <usb_driver_release_interface+0>:    push   %ebp
0x00000311 <usb_driver_release_interface+1>:    mov    %esp,%ebp
0x00000313 <usb_driver_release_interface+3>:    push   %ebx
0x00000314 <usb_driver_release_interface+4>:    mov    0xc(%ebp),%ecx
0x00000317 <usb_driver_release_interface+7>:    lea    0x10(%ecx),%ebx
0x0000031a <usb_driver_release_interface+10>:   mov    0x70(%ebx),%edx
0x0000031d <usb_driver_release_interface+13>:   test   %edx,%edx

hmm, iface = %ebp, dev = %ecx = 0x6b6b6b6b, how did that get
freed/poisoned since it's encapsulated by iface?

Ta,
	Zwane

[PATCH] fix different usb-serial oopses for 2.6.7

[Greg KH]

Hi,

Ok, thanks to both of you posting bug reports that seemed quite
different, I think I've finally fixed both of your issues.  The patch
below is what I've just added to my trees and will send to Linus in a
bit, and should solve both problems.

Basically the issue was 2 things:
	- Zwane correctly found that we shouldn't have been calling the
	  usb_driver_release_interface() call on disconnect, but if you
	  didn't make this call, we leaked memory.  This was because of
	  the next piece...
	- Harald noticed that if you unloaded a usb-serial driver with
	  the device still plugged in, and then removed it, the kernel
	  oopsed.  He also noticed double calls to the disconnect
	  function.  This was because we were incorrectly binding the
	  device to the usb serial generic driver instead of the one
	  that was controlling it.

So, by fixing the usb-serial generic issue, that fixed the fact that we
shouldn't have been calling the release_interface() call, as it isn't
necessary (the usb core will take care of it.)

Thanks to everyone for helping out here, and if with this patch, you
still have problems, please let me know...

/me crosses his fingers

greg k-h


USB: fix bug where removing usb-serial modules or usb serial devices could oops
 
This fixes the issue where the Generic driver would bind to all usb-serial
devices, so the disconnect would not properly go to the real driver that
controlled the device.  This was very bad when unloading the module with
the device still connected.

Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>

diff -Nru a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
--- a/drivers/usb/serial/generic.c	2004-06-29 14:32:36 -07:00
+++ b/drivers/usb/serial/generic.c	2004-06-29 14:32:36 -07:00
@@ -53,6 +53,32 @@
 	.num_ports =		1,
 	.shutdown =		usb_serial_generic_shutdown,
 };
+
+/* we want to look at all devices, as the vendor/product id can change
+ * depending on the command line argument */
+static struct usb_device_id generic_serial_ids[] = {
+	{.driver_info = 42},
+	{}
+};
+
+static int generic_probe(struct usb_interface *interface,
+			       const struct usb_device_id *id)
+{
+	const struct usb_device_id *id_pattern;
+
+	id_pattern = usb_match_id(interface, generic_device_ids);
+	if (id_pattern != NULL)
+		return usb_serial_probe(interface, id);
+	return -ENODEV;
+}
+
+static struct usb_driver generic_driver = {
+	.owner =	THIS_MODULE,
+	.name =		"usbserial_generic",
+	.probe =	generic_probe,
+	.disconnect =	usb_serial_disconnect,
+	.id_table =	generic_serial_ids,
+};
 #endif
 
 int usb_serial_generic_register (int _debug)
@@ -67,6 +93,12 @@
 
 	/* register our generic driver with ourselves */
 	retval = usb_serial_register (&usb_serial_generic_device);
+	if (retval)
+		goto exit;
+	retval = usb_register(&generic_driver);
+	if (retval)
+		usb_serial_deregister(&usb_serial_generic_device);
+exit:
 #endif
 	return retval;
 }
@@ -75,6 +107,7 @@
 {
 #ifdef CONFIG_USB_SERIAL_GENERIC
 	/* remove our generic driver */
+	usb_deregister(&generic_driver);
 	usb_serial_deregister (&usb_serial_generic_device);
 #endif
 }
diff -Nru a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
--- a/drivers/usb/serial/usb-serial.c	2004-06-29 14:32:36 -07:00
+++ b/drivers/usb/serial/usb-serial.c	2004-06-29 14:32:36 -07:00
@@ -355,25 +355,12 @@
 #define DRIVER_DESC "USB Serial Driver core"
 
 
-#ifdef CONFIG_USB_SERIAL_GENERIC
-/* we want to look at all devices, as the vendor/product id can change
- * depending on the command line argument */
-static struct usb_device_id generic_serial_ids[] = {
-	{.driver_info = 42},
-	{}
-};
-
-#endif /* CONFIG_USB_SERIAL_GENERIC */
-
 /* Driver structure we register with the USB core */
 static struct usb_driver usb_serial_driver = {
 	.owner =	THIS_MODULE,
 	.name =		"usbserial",
 	.probe =	usb_serial_probe,
 	.disconnect =	usb_serial_disconnect,
-#ifdef CONFIG_USB_SERIAL_GENERIC
-	.id_table =	generic_serial_ids,
-#endif
 };
 
 /* There is no MODULE_DEVICE_TABLE for usbserial.c.  Instead
@@ -1383,22 +1370,9 @@
 
 void usb_serial_deregister(struct usb_serial_device_type *device)
 {
-	struct usb_serial *serial;
-	int i;
-
 	info("USB Serial deregistering driver %s", device->name);
-
-	/* clear out the serial_table if the device is attached to a port */
-	for(i = 0; i < SERIAL_TTY_MINORS; ++i) {
-		serial = serial_table[i];
-		if ((serial != NULL) && (serial->type == device)) {
-			usb_driver_release_interface (&usb_serial_driver, serial->interface);
-			usb_serial_disconnect (serial->interface);
-		}
-	}
-
 	list_del(&device->driver_list);
-	usb_serial_bus_deregister (device);
+	usb_serial_bus_deregister(device);
 }
 
 

Re: [PATCH] fix different usb-serial oopses for 2.6.7

[Zwane Mwaikambo]

On Tue, 29 Jun 2004, Greg KH wrote:

> Ok, thanks to both of you posting bug reports that seemed quite
> different, I think I've finally fixed both of your issues.  The patch
> below is what I've just added to my trees and will send to Linus in a
> bit, and should solve both problems.
>
> Basically the issue was 2 things:
> 	- Zwane correctly found that we shouldn't have been calling the
> 	  usb_driver_release_interface() call on disconnect, but if you
> 	  didn't make this call, we leaked memory.  This was because of
> 	  the next piece...
> 	- Harald noticed that if you unloaded a usb-serial driver with
> 	  the device still plugged in, and then removed it, the kernel
> 	  oopsed.  He also noticed double calls to the disconnect
> 	  function.  This was because we were incorrectly binding the
> 	  device to the usb serial generic driver instead of the one
> 	  that was controlling it.
>
> So, by fixing the usb-serial generic issue, that fixed the fact that we
> shouldn't have been calling the release_interface() call, as it isn't
> necessary (the usb core will take care of it.)
>
> Thanks to everyone for helping out here, and if with this patch, you
> still have problems, please let me know...

Great, thanks Greg, this one works for me.