Doing port specific cleanup in the .port_remove hook is a
lot simpler and safer than doing it in the USB driver
.release or .disconnect methods. The removal of the port
from the usb-serial bus will happen before the USB driver
cleanup, so we must be careful about accessing port specific
driver data from any USB driver functions.
This problem surfaced after the commit
0998d0631 device-core: Ensure drvdata = NULL when no driver is bound
which turned the previous unsafe access into a reliable NULL
pointer dereference.
Fixes the following Oops:
[ 243.148471] BUG: unable to handle kernel NULL pointer dereference at (null)
[ 243.148508] IP: [<ffffffffa0468527>] stop_read_write_urbs+0x37/0x80 [usb_wwan]
[ 243.148556] PGD 79d60067 PUD 79d61067 PMD 0
[ 243.148590] Oops: 0000 [#1] SMP
[ 243.148617] Modules linked in: sr_mod cdrom qmi_wwan usbnet option cdc_wdm usb_wwan usbserial usb_storage uas fuse af_packet ip6table_filter ip6_tables iptable_filter ip_tables x_tables tun edd
cpufreq_conservative cpufreq_userspace cpufreq_powersave snd_pcm_oss snd_mixer_oss acpi_cpufreq snd_seq mperf snd_seq_device coretemp arc4 sg hp_wmi sparse_keymap uvcvideo videobuf2_core
videodev videobuf2_vmalloc videobuf2_memops rtl8192ce rtl8192c_common rtlwifi joydev pcspkr microcode mac80211 i2c_i801 lpc_ich r8169 snd_hda_codec_idt cfg80211 snd_hda_intel snd_hda_codec rfkill
snd_hwdep snd_pcm wmi snd_timer ac snd soundcore snd_page_alloc battery uhci_hcd i915 drm_kms_helper drm i2c_algo_bit ehci_hcd thermal usbcore video usb_common button processor thermal_sys
[ 243.149007] CPU 1
[ 243.149027] Pid: 135, comm: khubd Not tainted 3.5.0-rc7-next-20120720-1-vanilla #1 Hewlett-Packard HP Mini 110-3700 /1584
[ 243.149072] RIP: 0010:[<ffffffffa0468527>] [<ffffffffa0468527>] stop_read_write_urbs+0x37/0x80 [usb_wwan]
[ 243.149118] RSP: 0018:ffff880037e75b30 EFLAGS: 00010286
[ 243.149133] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff88005912aa28
[ 243.149150] RDX: ffff88005e95f028 RSI: 0000000000000000 RDI: ffff88005f7c1a10
[ 243.149166] RBP: ffff880037e75b60 R08: 0000000000000000 R09: ffffffff812cea90
[ 243.149182] R10: 0000000000000000 R11: 0000000000000001 R12: ffff88006539b440
[ 243.149198] R13: ffff88006539b440 R14: 0000000000000000 R15: 0000000000000000
[ 243.149216] FS: 0000000000000000(0000) GS:ffff88007ee80000(0000) knlGS:0000000000000000
[ 243.149233] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 243.149248] CR2: 0000000000000000 CR3: 0000000079fe0000 CR4: 00000000000007e0
[ 243.149264] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 243.149280] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 243.149298] Process khubd (pid: 135, threadinfo ffff880037e74000, task ffff880037d40600)
[ 243.149313] Stack:
[ 243.149323] ffff880037e75b40 ffff88006539b440 ffff8800799bc830 ffff88005f7c1800
[ 243.149348] 0000000000000001 ffff88006539b448 ffff880037e75b70 ffffffffa04685e9
[ 243.149371] ffff880037e75bc0 ffffffffa0473765 ffff880037354988 ffff88007b594800
[ 243.149395] Call Trace:
[ 243.149419] [<ffffffffa04685e9>] usb_wwan_disconnect+0x9/0x10 [usb_wwan]
[ 243.149447] [<ffffffffa0473765>] usb_serial_disconnect+0xd5/0x120 [usbserial]
[ 243.149511] [<ffffffffa0046b48>] usb_unbind_interface+0x58/0x1a0 [usbcore]
[ 243.149545] [<ffffffff8139ebd7>] __device_release_driver+0x77/0xe0
[ 243.149567] [<ffffffff8139ec67>] device_release_driver+0x27/0x40
[ 243.149587] [<ffffffff8139e5cf>] bus_remove_device+0xdf/0x150
[ 243.149608] [<ffffffff8139bc78>] device_del+0x118/0x1a0
[ 243.149661] [<ffffffffa0044590>] usb_disable_device+0xb0/0x280 [usbcore]
[ 243.149718] [<ffffffffa003c6fd>] usb_disconnect+0x9d/0x140 [usbcore]
[ 243.149770] [<ffffffffa003da7d>] hub_port_connect_change+0xad/0x8a0 [usbcore]
[ 243.149825] [<ffffffffa0043bf5>] ? usb_control_msg+0xe5/0x110 [usbcore]
[ 243.149878] [<ffffffffa003e6e3>] hub_events+0x473/0x760 [usbcore]
[ 243.149931] [<ffffffffa003ea05>] hub_thread+0x35/0x1d0 [usbcore]
[ 243.149955] [<ffffffff81061960>] ? add_wait_queue+0x60/0x60
[ 243.150004] [<ffffffffa003e9d0>] ? hub_events+0x760/0x760 [usbcore]
[ 243.150026] [<ffffffff8106133e>] kthread+0x8e/0xa0
[ 243.150047] [<ffffffff8157ec04>] kernel_thread_helper+0x4/0x10
[ 243.150068] [<ffffffff810612b0>] ? flush_kthread_work+0x120/0x120
[ 243.150088] [<ffffffff8157ec00>] ? gs_change+0xb/0xb
[ 243.150101] Code: fd 41 54 53 48 83 ec 08 80 7f 1a 00 74 57 49 89 fc 31 db 90 49 8b 7c 24 20 45 31 f6 48 81 c7 10 02 00 00 e8 bc 64 f3 e0 49 89 c7 <4b> 8b 3c 37 49 83 c6 08 e8 4c a5 bd ff 49 83 fe 20
75 ed 45 30
[ 243.150257] RIP [<ffffffffa0468527>] stop_read_write_urbs+0x37/0x80 [usb_wwan]
[ 243.150282] RSP <ffff880037e75b30>
[ 243.150294] CR2: 0000000000000000
[ 243.177170] ---[ end trace fba433d9015ffb8c ]---
Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
Reported-by: Thomas Schäfer <tschaefer@xxxxxxxxxxx>
Suggested-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
Signed-off-by: Bjørn Mork <bjorn@xxxxxxx>
---
drivers/usb/serial/ipw.c | 3 +-
drivers/usb/serial/option.c | 4 +--
drivers/usb/serial/qcserial.c | 5 ++--
drivers/usb/serial/usb-wwan.h | 3 +-
drivers/usb/serial/usb_wwan.c | 64 +++++++++++++++++------------------------
5 files changed, 31 insertions(+), 48 deletions(-)
diff --git a/drivers/usb/serial/ipw.c b/drivers/usb/serial/ipw.c
index 5811d34..2cb30c5 100644
--- a/drivers/usb/serial/ipw.c
+++ b/drivers/usb/serial/ipw.c
@@ -227,7 +227,6 @@ static void ipw_release(struct usb_serial *serial)
{
struct usb_wwan_intf_private *data = usb_get_serial_data(serial);
- usb_wwan_release(serial);
usb_set_serial_data(serial, NULL);
kfree(data);
}
@@ -309,12 +308,12 @@ static struct usb_serial_driver ipw_device = {
.description = "IPWireless converter",
.id_table = id_table,
.num_ports = 1,
- .disconnect = usb_wwan_disconnect,
.open = ipw_open,
.close = ipw_close,
.probe = ipw_probe,
.attach = usb_wwan_startup,
.release = ipw_release,
+ .port_remove = usb_wwan_port_remove,
.dtr_rts = ipw_dtr_rts,
.write = usb_wwan_write,
};
diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
index 08ff9b8..2f3f917 100644
--- a/drivers/usb/serial/option.c
+++ b/drivers/usb/serial/option.c
@@ -1297,8 +1297,8 @@ static struct usb_serial_driver option_1port_device = {
.tiocmset = usb_wwan_tiocmset,
.ioctl = usb_wwan_ioctl,
.attach = usb_wwan_startup,
- .disconnect = usb_wwan_disconnect,
.release = option_release,
+ .port_remove = usb_wwan_port_remove,
.read_int_callback = option_instat_callback,
#ifdef CONFIG_PM
.suspend = usb_wwan_suspend,
@@ -1414,8 +1414,6 @@ static void option_release(struct usb_serial *serial)
struct usb_wwan_intf_private *intfdata = usb_get_serial_data(serial);
struct option_private *priv = intfdata->private;
- usb_wwan_release(serial);
-
kfree(priv);
kfree(intfdata);
}
diff --git a/drivers/usb/serial/qcserial.c b/drivers/usb/serial/qcserial.c
index 8d10301..314ae8c 100644
--- a/drivers/usb/serial/qcserial.c
+++ b/drivers/usb/serial/qcserial.c
@@ -262,8 +262,7 @@ static void qc_release(struct usb_serial *serial)
{
struct usb_wwan_intf_private *priv = usb_get_serial_data(serial);
- /* Call usb_wwan release & free the private data allocated in qcprobe */
- usb_wwan_release(serial);
+ /* Free the private data allocated in qcprobe */
usb_set_serial_data(serial, NULL);
kfree(priv);
}
@@ -283,8 +282,8 @@ static struct usb_serial_driver qcdevice = {
.write_room = usb_wwan_write_room,
.chars_in_buffer = usb_wwan_chars_in_buffer,
.attach = usb_wwan_startup,
- .disconnect = usb_wwan_disconnect,
.release = qc_release,
+ .port_remove = usb_wwan_port_remove,
#ifdef CONFIG_PM
.suspend = usb_wwan_suspend,
.resume = usb_wwan_resume,
diff --git a/drivers/usb/serial/usb-wwan.h b/drivers/usb/serial/usb-wwan.h
index c47b6ec..1f034d2 100644
--- a/drivers/usb/serial/usb-wwan.h
+++ b/drivers/usb/serial/usb-wwan.h
@@ -9,8 +9,7 @@ extern void usb_wwan_dtr_rts(struct usb_serial_port *port, int on);
extern int usb_wwan_open(struct tty_struct *tty, struct usb_serial_port *port);
extern void usb_wwan_close(struct usb_serial_port *port);
extern int usb_wwan_startup(struct usb_serial *serial);
-extern void usb_wwan_disconnect(struct usb_serial *serial);
-extern void usb_wwan_release(struct usb_serial *serial);
+extern int usb_wwan_port_remove(struct usb_serial_port *port);
extern int usb_wwan_write_room(struct tty_struct *tty);
extern void usb_wwan_set_termios(struct tty_struct *tty,
struct usb_serial_port *port,
diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
index f35971d..7d08113 100644
--- a/drivers/usb/serial/usb_wwan.c
+++ b/drivers/usb/serial/usb_wwan.c
@@ -565,62 +565,50 @@ bail_out_error:
}
EXPORT_SYMBOL(usb_wwan_startup);
-static void stop_read_write_urbs(struct usb_serial *serial)
+int usb_wwan_port_remove(struct usb_serial_port *port)
{
- int i, j;
- struct usb_serial_port *port;
+ int i;
struct usb_wwan_port_private *portdata;
- /* Stop reading/writing urbs */
- for (i = 0; i < serial->num_ports; ++i) {
- port = serial->port[i];
- portdata = usb_get_serial_port_data(port);
- for (j = 0; j < N_IN_URB; j++)
- usb_kill_urb(portdata->in_urbs[j]);
- for (j = 0; j < N_OUT_URB; j++)
- usb_kill_urb(portdata->out_urbs[j]);
+ portdata = usb_get_serial_port_data(port);
+ usb_set_serial_port_data(port, NULL);
+
+ /* Stop reading/writing urbs and free them */
+ for (i = 0; i < N_IN_URB; i++) {
+ usb_kill_urb(portdata->in_urbs[i]);
+ usb_free_urb(portdata->in_urbs[i]);
+ free_page((unsigned long)portdata->in_buffer[i]);
+ }
+ for (i = 0; i < N_OUT_URB; i++) {
+ usb_kill_urb(portdata->out_urbs[i]);
+ usb_free_urb(portdata->out_urbs[i]);
+ kfree(portdata->out_buffer[i]);
}
-}
-void usb_wwan_disconnect(struct usb_serial *serial)
-{
- stop_read_write_urbs(serial);
+ /* Now free port private data */
+ kfree(portdata);
+ return 0;
}
-EXPORT_SYMBOL(usb_wwan_disconnect);
+EXPORT_SYMBOL(usb_wwan_port_remove);
-void usb_wwan_release(struct usb_serial *serial)
+#ifdef CONFIG_PM
+static void stop_read_write_urbs(struct usb_serial *serial)
{
int i, j;
struct usb_serial_port *port;
struct usb_wwan_port_private *portdata;
- /* Now free them */
+ /* Stop reading/writing urbs */
for (i = 0; i < serial->num_ports; ++i) {
port = serial->port[i];
portdata = usb_get_serial_port_data(port);
-
- for (j = 0; j < N_IN_URB; j++) {
- usb_free_urb(portdata->in_urbs[j]);
- free_page((unsigned long)
- portdata->in_buffer[j]);
- portdata->in_urbs[j] = NULL;
- }
- for (j = 0; j < N_OUT_URB; j++) {
- usb_free_urb(portdata->out_urbs[j]);
- kfree(portdata->out_buffer[j]);
- portdata->out_urbs[j] = NULL;
- }
- }
-
- /* Now free per port private data */
- for (i = 0; i < serial->num_ports; i++) {
- port = serial->port[i];
- kfree(usb_get_serial_port_data(port));
+ for (j = 0; j < N_IN_URB; j++)
+ usb_kill_urb(portdata->in_urbs[j]);
+ for (j = 0; j < N_OUT_URB; j++)
+ usb_kill_urb(portdata->out_urbs[j]);
}
}
-EXPORT_SYMBOL(usb_wwan_release);
-#ifdef CONFIG_PM
int usb_wwan_suspend(struct usb_serial *serial, pm_message_t message)
{
struct usb_wwan_intf_private *intfdata = serial->private;
--
1.7.10.4
--
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
