| Message ID | CA+v9cxawVwKakF6c_RpAw2XUGWcbqd8M+ZJqyq76Au9rmNosmQ@mail.gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Apr 20, 2012 at 9:37 PM, Huajun Li <huajun.li.lee@gmail.com> wrote: > > Above patch has already been integrated to mainline. However, maybe > there still exists another potentail use-after-free issue, here is a > case: > After release the lock in unlink_urbs(), defer_bh() may move > current skb from rxq/txq to dev->done queue, even cause the skb be > released. Then in next loop cycle, it can't refer to expected skb, and > may Oops again. Could you explain in a bit detail? Why can't the expected skb be refered to in next loop? > > To easily reproduce it, in unlink_urbs(), you can delay a short time > after usb_put_urb(urb), then disconnect your device while transferring > data, and repeat it times you will find errors on your screen. Could you post out the error log? > > Following is a draft patch to guarantee the queue consistent, and > refer to expected skb in each loop cycle: > > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > index b7b3f5b..6da0141 100644 > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -578,16 +578,28 @@ EXPORT_SYMBOL_GPL(usbnet_purge_paused_rxq); > static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q) > { > unsigned long flags; > - struct sk_buff *skb, *skbnext; > + struct sk_buff *skb; > int count = 0; > > spin_lock_irqsave (&q->lock, flags); > - skb_queue_walk_safe(q, skb, skbnext) { > + while (!skb_queue_empty(q)) { > struct skb_data *entry; > struct urb *urb; > int retval; > > - entry = (struct skb_data *) skb->cb; > + skb_queue_walk(q, skb) { > + entry = (struct skb_data *)skb->cb; > + if (entry->state == rx_done || > + entry->state == tx_done || > + entry->state == rx_cleanup) Maybe it is not necessary, if the state has been changed to rx_done or tx_done or rx_cleanup, it means the URB referenced to by the skb has been completed, and the coming usb_unlink_urb can handle the case correctly. > + continue; > + else > + break; > + } > + > + if (skb == (struct sk_buff *)(q)) > + break; > + > urb = entry->urb; > > > Thanks, > --Huajun Li Thanks,
On Fri, Apr 20, 2012 at 10:22 PM, Ming Lei <tom.leiming@gmail.com> wrote: > On Fri, Apr 20, 2012 at 9:37 PM, Huajun Li <huajun.li.lee@gmail.com> wrote: >> >> Above patch has already been integrated to mainline. However, maybe >> there still exists another potentail use-after-free issue, here is a >> case: >> After release the lock in unlink_urbs(), defer_bh() may move >> current skb from rxq/txq to dev->done queue, even cause the skb be >> released. Then in next loop cycle, it can't refer to expected skb, and >> may Oops again. > > Could you explain in a bit detail? Why can't the expected skb be refered > to in next loop? unlink_urbs() complete handler -------------------------------------- ------------------------------------------------- spin_unlock_irqrestore() rx_complete() derver_bh() __skb_unlink() __skb_queue_tail(&dev->done, skb) =======> skb is moved to dev->done, and can be freed by usbnet_bh() skb_queue_walk_safe() tmp = skb->next ===> refer to freed skb > >> >> To easily reproduce it, in unlink_urbs(), you can delay a short time >> after usb_put_urb(urb), then disconnect your device while transferring >> data, and repeat it times you will find errors on your screen. > > Could you post out the error log? The log like this: =============================================================== [45219.230127] general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC [45219.230192] CPU 1 [45219.230208] Modules linked in: cdc_ether usbnet(O) bluetooth dm_crypt binfmt_misc snd_hda_codec_analog snd_hda_intel snd_hda_codec snd_hwdep snd_pcm snd_seq_midi snd_rawmidi snd_seq_midi_event hp_wmi ppdev sparse_keymap i915 snd_seq snd_timer coretemp microcode snd_seq_device psmouse tpm_infineon serio_raw snd soundcore drm_kms_helper snd_page_alloc tpm_tis tpm parport_pc tpm_bios drm i2c_algo_bit video lp parport sg ehci_hcd uhci_hcd sr_mod sd_mod usbcore e1000e usb_common floppy [45219.230658] [45219.230673] Pid: 200, comm: khubd Tainted: G IO 3.4.0-rc3 #56 Hewlett-Packard HP Compaq dc7800p Convertible Minitower/0AACh [45219.230761] RIP: 0010:[<ffffffffa0094e7d>] [<ffffffffa0094e7d>] usb_get_urb+0x1d/0x70 [usbcore] [45219.230837] RSP: 0018:ffff8800554df8e0 EFLAGS: 00010002 [45219.230874] RAX: 6b6b6b6b6b6b6b6b RBX: 6b6b6b6b6b6b6b6b RCX: 0000000000008760 [45219.230920] RDX: 6b6b6b6b6b6b6b6b RSI: 0000000000000202 RDI: 6b6b6b6b6b6b6b6b [45219.230966] RBP: ffff8800554df8f0 R08: 0000000304b1e1be R09: 0000000000000001 [45219.231012] R10: 0000000000000001 R11: 0000000000000000 R12: 6b6b6b6b6b6b6b6b [45219.231058] R13: ffff88000650e330 R14: ffff88000650e318 R15: 0000000000000001 [45219.231105] FS: 0000000000000000(0000) GS:ffff88007a400000(0000) knlGS:0000000000000000 [45219.231158] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b [45219.231196] CR2: 00007f1bd331c3e0 CR3: 000000000656b000 CR4: 00000000000007e0 [45219.231243] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [45219.231289] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 [45219.231335] Process khubd (pid: 200, threadinfo ffff8800554de000, task ffff880055b4a410) [45219.231387] Stack: [45219.231405] ffff8800567daf40 ffff88000650e330 ffff8800554df940 ffffffffa053ee1a [45219.231469] 0000000000000202 ffff88000650e2a0 ffff8800554df940 ffff88000650e140 [45219.231534] ffff880055b4a410 ffff88000650e318 ffff88000650e378 ffff88000650e2a0 [45219.231597] Call Trace: [45219.231622] [<ffffffffa053ee1a>] unlink_urbs.isra.14+0xca/0x170 [usbnet] [45219.231670] [<ffffffffa053f05b>] usbnet_terminate_urbs+0x19b/0x350 [usbnet] [45219.231721] [<ffffffff810d1700>] ? try_to_wake_up+0x580/0x580 [45219.231762] [<ffffffff810c24ba>] ? __wake_up+0x3a/0x90 [45219.231801] [<ffffffffa053ff90>] usbnet_stop+0x190/0x270 [usbnet] [45219.231846] [<ffffffff817b1226>] __dev_close_many+0xd6/0x160 [45219.231886] [<ffffffff817b1368>] dev_close_many+0xb8/0x160 [45219.231926] [<ffffffff817b1518>] rollback_registered_many+0x108/0x390 [45219.231972] [<ffffffff817b1874>] rollback_registered+0x44/0x70 [45219.232013] [<ffffffff817b1920>] unregister_netdevice_queue+0x80/0xf0 [45219.232058] [<ffffffff817b1ae0>] unregister_netdev+0x30/0x50 [45219.232100] [<ffffffffa053e61a>] usbnet_disconnect+0x8a/0x170 [usbnet] [45219.232155] [<ffffffffa009adc5>] usb_unbind_interface+0x65/0x230 [usbcore] [45219.232205] [<ffffffff8163cda7>] __device_release_driver+0x157/0x170 [45219.232249] [<ffffffff8163cdfe>] device_release_driver+0x3e/0x60 [45219.232291] [<ffffffff8163c27f>] bus_remove_device+0x15f/0x210 [45219.232331] [<ffffffff81637e6d>] device_del+0x1ad/0x2b0 [45219.232379] [<ffffffffa0097970>] usb_disable_device+0x100/0x340 [usbcore] [45219.232435] [<ffffffffa008a417>] usb_disconnect+0xf7/0x220 [usbcore] [45219.232487] [<ffffffffa008e411>] hub_thread+0x1321/0x21e0 [usbcore] [45219.232535] [<ffffffff810b3780>] ? wake_up_bit+0x50/0x50 [45219.232581] [<ffffffffa008d0f0>] ? usb_remote_wakeup+0xb0/0xb0 [usbcore] [45219.232628] [<ffffffff810b2b9f>] kthread+0xdf/0xf0 [45219.232664] [<ffffffff819a5574>] kernel_thread_helper+0x4/0x10 [45219.232706] [<ffffffff819970b0>] ? retint_restore_args+0x13/0x13 [45219.232749] [<ffffffff810b2ac0>] ? __init_kthread_worker+0x80/0x80 [45219.232791] [<ffffffff819a5570>] ? gs_change+0x13/0x13 [45219.232826] Code: ff ff e9 d4 fb ff ff 0f 1f 80 00 00 00 00 55 48 89 e5 48 83 ec 10 66 66 66 66 90 48 83 05 2b aa 02 00 01 48 85 ff 48 89 f8 74 19 <8b> 17 48 83 05 21 aa 02 00 01 85 d2 74 0d f0 ff 00 48 83 05 2a [45219.233184] RIP [<ffffffffa0094e7d>] usb_get_urb+0x1d/0x70 [usbcore] [45219.233240] RSP <ffff8800554df8e0> [45219.233268] ---[ end trace a7919e7f17c0a727 ]--- [45222.870031] usb0: no IPv6 routers present > >> >> Following is a draft patch to guarantee the queue consistent, and >> refer to expected skb in each loop cycle: >> >> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c >> index b7b3f5b..6da0141 100644 >> --- a/drivers/net/usb/usbnet.c >> +++ b/drivers/net/usb/usbnet.c >> @@ -578,16 +578,28 @@ EXPORT_SYMBOL_GPL(usbnet_purge_paused_rxq); >> static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q) >> { >> unsigned long flags; >> - struct sk_buff *skb, *skbnext; >> + struct sk_buff *skb; >> int count = 0; >> >> spin_lock_irqsave (&q->lock, flags); >> - skb_queue_walk_safe(q, skb, skbnext) { >> + while (!skb_queue_empty(q)) { >> struct skb_data *entry; >> struct urb *urb; >> int retval; >> >> - entry = (struct skb_data *) skb->cb; >> + skb_queue_walk(q, skb) { >> + entry = (struct skb_data *)skb->cb; >> + if (entry->state == rx_done || >> + entry->state == tx_done || >> + entry->state == rx_cleanup) > > Maybe it is not necessary, if the state has been changed to rx_done > or tx_done or rx_cleanup, it means the URB referenced to by the skb > has been completed, and the coming usb_unlink_urb can handle the > case correctly. > If its state is x_done/tx_done/rx_cleanup, that means the the skb will be released soon, right? If so, it should avoid calling usb_unlink_urb(). >> + continue; >> + else >> + break; >> + } >> + >> + if (skb == (struct sk_buff *)(q)) >> + break; >> + >> urb = entry->urb; >> >> >> Thanks, >> --Huajun Li > > > Thanks, > -- > Ming Lei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 20, 2012 at 10:56 PM, Huajun Li <huajun.li.lee@gmail.com> wrote: > On Fri, Apr 20, 2012 at 10:22 PM, Ming Lei <tom.leiming@gmail.com> wrote: >> On Fri, Apr 20, 2012 at 9:37 PM, Huajun Li <huajun.li.lee@gmail.com> wrote: >>> >>> Above patch has already been integrated to mainline. However, maybe >>> there still exists another potentail use-after-free issue, here is a >>> case: >>> After release the lock in unlink_urbs(), defer_bh() may move >>> current skb from rxq/txq to dev->done queue, even cause the skb be >>> released. Then in next loop cycle, it can't refer to expected skb, and >>> may Oops again. >> >> Could you explain in a bit detail? Why can't the expected skb be refered >> to in next loop? > > > unlink_urbs() complete handler > -------------------------------------- > ------------------------------------------------- > spin_unlock_irqrestore() > rx_complete() > derver_bh() > > __skb_unlink() > > __skb_queue_tail(&dev->done, skb) =======> skb is moved to > dev->done, and can be freed by usbnet_bh() > skb_queue_walk_safe() > tmp = skb->next ===> refer to freed skb > Sorry, email client messed up these lines, resend it: unlink_urbs() complete handler ------------------------ ------------------------------ spin_unlock_irqrestore() rx_complete() derver_bh() __skb_unlink() __skb_queue_tail(&dev->done, skb) =======> skb is moved to dev->done, and can be freed by usbnet_bh() skb_queue_walk_safe() tmp = skb->next ===> refer to freed skb -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index b7b3f5b..6da0141 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -578,16 +578,28 @@ EXPORT_SYMBOL_GPL(usbnet_purge_paused_rxq); static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q) { unsigned long flags; - struct sk_buff *skb, *skbnext; + struct sk_buff *skb; int count = 0; spin_lock_irqsave (&q->lock, flags); - skb_queue_walk_safe(q, skb, skbnext) { + while (!skb_queue_empty(q)) { struct skb_data *entry; struct urb *urb; int retval; - entry = (struct skb_data *) skb->cb; + skb_queue_walk(q, skb) { + entry = (struct skb_data *)skb->cb; + if (entry->state == rx_done || + entry->state == tx_done || + entry->state == rx_cleanup) + continue; + else + break; + } + + if (skb == (struct sk_buff *)(q)) + break; + urb = entry->urb;