| Message ID | 20220409120901.267526-1-dzm91@hust.edu.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | driver: usb: nullify dangling pointer in cdc_ncm_free | expand |
On Sat, Apr 09, 2022 at 08:09:00PM +0800, Dongliang Mu wrote: > From: Dongliang Mu <mudongliangabcd@gmail.com> > > cdc_ncm_bind calls cdc_ncm_bind_common and sets dev->data[0] > with ctx. However, in the unbind function - cdc_ncm_unbind, > it calls cdc_ncm_free and frees ctx, leaving dev->data[0] as > a dangling pointer. The following ioctl operation will trigger > the UAF in the function cdc_ncm_set_dgram_size. > > Fix this by setting dev->data[0] as zero. This sounds like a poor band-aid. Please explain how this prevent the ioctl() from racing with unbind(). Johan > ================================================================== > BUG: KASAN: use-after-free in cdc_ncm_set_dgram_size+0xc91/0xde0 > Read of size 8 at addr ffff8880755210b0 by task dhcpcd/3174 > > Call Trace: > <TASK> > __dump_stack lib/dump_stack.c:88 [inline] > dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106 > print_address_description.constprop.0.cold+0xeb/0x495 mm/kasan/report.c:313 > print_report mm/kasan/report.c:429 [inline] > kasan_report.cold+0xf4/0x1c6 mm/kasan/report.c:491 > cdc_ncm_set_dgram_size+0xc91/0xde0 drivers/net/usb/cdc_ncm.c:608 > cdc_ncm_change_mtu+0x10c/0x140 drivers/net/usb/cdc_ncm.c:798 > __dev_set_mtu net/core/dev.c:8519 [inline] > dev_set_mtu_ext+0x352/0x5b0 net/core/dev.c:8572 > dev_set_mtu+0x8e/0x120 net/core/dev.c:8596 > dev_ifsioc+0xb87/0x1090 net/core/dev_ioctl.c:332 > dev_ioctl+0x1b9/0xe30 net/core/dev_ioctl.c:586 > sock_do_ioctl+0x15a/0x230 net/socket.c:1136 > sock_ioctl+0x2f1/0x640 net/socket.c:1239 > vfs_ioctl fs/ioctl.c:51 [inline] > __do_sys_ioctl fs/ioctl.c:870 [inline] > __se_sys_ioctl fs/ioctl.c:856 [inline] > __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:856 > do_syscall_x64 arch/x86/entry/common.c:50 [inline] > do_syscall_64+0x35/0x80 arch/x86/entry/common.c:80 > entry_SYSCALL_64_after_hwframe+0x44/0xae > RIP: 0033:0x7f00859e70e7 > RSP: 002b:00007ffedd503dd8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 > RAX: ffffffffffffffda RBX: 00007f00858f96c8 RCX: 00007f00859e70e7 > RDX: 00007ffedd513fc8 RSI: 0000000000008922 RDI: 0000000000000018 > RBP: 00007ffedd524178 R08: 00007ffedd513f88 R09: 00007ffedd513f38 > R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000 > R13: 00007ffedd513fc8 R14: 0000000000000028 R15: 0000000000008922 > </TASK> > Reported-by: syzbot+eabbf2aaa999cc507108@syzkaller.appspotmail.com > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > --- > drivers/net/usb/cdc_ncm.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c > index 15f91d691bba..9fc2df9f0b63 100644 > --- a/drivers/net/usb/cdc_ncm.c > +++ b/drivers/net/usb/cdc_ncm.c > @@ -1019,6 +1019,7 @@ void cdc_ncm_unbind(struct usbnet *dev, struct usb_interface *intf) > > usb_set_intfdata(intf, NULL); > cdc_ncm_free(ctx); > + dev->data[0] = 0; > } > EXPORT_SYMBOL_GPL(cdc_ncm_unbind);
On Sun, Apr 10, 2022 at 5:14 AM Dongliang Mu <dzm91@hust.edu.cn> wrote: > > From: Dongliang Mu <mudongliangabcd@gmail.com> > > cdc_ncm_bind calls cdc_ncm_bind_common and sets dev->data[0] > with ctx. However, in the unbind function - cdc_ncm_unbind, > it calls cdc_ncm_free and frees ctx, leaving dev->data[0] as > a dangling pointer. The following ioctl operation will trigger > the UAF in the function cdc_ncm_set_dgram_size. First of all, please use the standard form of referring to the func() as in this sentence. > Fix this by setting dev->data[0] as zero. > > ================================================================== > BUG: KASAN: use-after-free in cdc_ncm_set_dgram_size+0xc91/0xde0 > Read of size 8 at addr ffff8880755210b0 by task dhcpcd/3174 > Please, avoid SO noisy commit messages. Find the core part of the traceback(s) which should be rarely more than 5-10 lines. ... The code seems fine.
On Mon, Apr 11, 2022 at 8:14 PM Johan Hovold <johan@kernel.org> wrote: > > On Sat, Apr 09, 2022 at 08:09:00PM +0800, Dongliang Mu wrote: > > From: Dongliang Mu <mudongliangabcd@gmail.com> > > > > cdc_ncm_bind calls cdc_ncm_bind_common and sets dev->data[0] > > with ctx. However, in the unbind function - cdc_ncm_unbind, > > it calls cdc_ncm_free and frees ctx, leaving dev->data[0] as > > a dangling pointer. The following ioctl operation will trigger > > the UAF in the function cdc_ncm_set_dgram_size. > > > > Fix this by setting dev->data[0] as zero. > > This sounds like a poor band-aid. Please explain how this prevent the > ioctl() from racing with unbind(). You mean the following thread interlaving? ioctl unbind cdc_ncm_free(ctx); dev->data[0] dev->data[0] = 0; It seems this will still trigger UAF. Maybe we need to add mutex to prevent this. But I am not sure. > > Johan > > > ================================================================== > > BUG: KASAN: use-after-free in cdc_ncm_set_dgram_size+0xc91/0xde0 > > Read of size 8 at addr ffff8880755210b0 by task dhcpcd/3174 > > > > Call Trace: > > <TASK> > > __dump_stack lib/dump_stack.c:88 [inline] > > dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106 > > print_address_description.constprop.0.cold+0xeb/0x495 mm/kasan/report.c:313 > > print_report mm/kasan/report.c:429 [inline] > > kasan_report.cold+0xf4/0x1c6 mm/kasan/report.c:491 > > cdc_ncm_set_dgram_size+0xc91/0xde0 drivers/net/usb/cdc_ncm.c:608 > > cdc_ncm_change_mtu+0x10c/0x140 drivers/net/usb/cdc_ncm.c:798 > > __dev_set_mtu net/core/dev.c:8519 [inline] > > dev_set_mtu_ext+0x352/0x5b0 net/core/dev.c:8572 > > dev_set_mtu+0x8e/0x120 net/core/dev.c:8596 > > dev_ifsioc+0xb87/0x1090 net/core/dev_ioctl.c:332 > > dev_ioctl+0x1b9/0xe30 net/core/dev_ioctl.c:586 > > sock_do_ioctl+0x15a/0x230 net/socket.c:1136 > > sock_ioctl+0x2f1/0x640 net/socket.c:1239 > > vfs_ioctl fs/ioctl.c:51 [inline] > > __do_sys_ioctl fs/ioctl.c:870 [inline] > > __se_sys_ioctl fs/ioctl.c:856 [inline] > > __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:856 > > do_syscall_x64 arch/x86/entry/common.c:50 [inline] > > do_syscall_64+0x35/0x80 arch/x86/entry/common.c:80 > > entry_SYSCALL_64_after_hwframe+0x44/0xae > > RIP: 0033:0x7f00859e70e7 > > RSP: 002b:00007ffedd503dd8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 > > RAX: ffffffffffffffda RBX: 00007f00858f96c8 RCX: 00007f00859e70e7 > > RDX: 00007ffedd513fc8 RSI: 0000000000008922 RDI: 0000000000000018 > > RBP: 00007ffedd524178 R08: 00007ffedd513f88 R09: 00007ffedd513f38 > > R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000 > > R13: 00007ffedd513fc8 R14: 0000000000000028 R15: 0000000000008922 > > </TASK> > > > Reported-by: syzbot+eabbf2aaa999cc507108@syzkaller.appspotmail.com > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > --- > > drivers/net/usb/cdc_ncm.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c > > index 15f91d691bba..9fc2df9f0b63 100644 > > --- a/drivers/net/usb/cdc_ncm.c > > +++ b/drivers/net/usb/cdc_ncm.c > > @@ -1019,6 +1019,7 @@ void cdc_ncm_unbind(struct usbnet *dev, struct usb_interface *intf) > > > > usb_set_intfdata(intf, NULL); > > cdc_ncm_free(ctx); > > + dev->data[0] = 0; > > } > > EXPORT_SYMBOL_GPL(cdc_ncm_unbind);
On Mon, Apr 11, 2022 at 10:55 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Sun, Apr 10, 2022 at 5:14 AM Dongliang Mu <dzm91@hust.edu.cn> wrote: > > > > From: Dongliang Mu <mudongliangabcd@gmail.com> > > > > cdc_ncm_bind calls cdc_ncm_bind_common and sets dev->data[0] > > with ctx. However, in the unbind function - cdc_ncm_unbind, > > it calls cdc_ncm_free and frees ctx, leaving dev->data[0] as > > a dangling pointer. The following ioctl operation will trigger > > the UAF in the function cdc_ncm_set_dgram_size. > > First of all, please use the standard form of referring to the func() > as in this sentence. OK, no problem. > > > Fix this by setting dev->data[0] as zero. > > > > ================================================================== > > BUG: KASAN: use-after-free in cdc_ncm_set_dgram_size+0xc91/0xde0 > > Read of size 8 at addr ffff8880755210b0 by task dhcpcd/3174 > > > > Please, avoid SO noisy commit messages. Find the core part of the > traceback(s) which should be rarely more than 5-10 lines. Sure. I will revise them in the v2 patch. > > ... > > The code seems fine. > > -- > With Best Regards, > Andy Shevchenko
On Thu, Apr 14, 2022 at 9:58 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > On Mon, Apr 11, 2022 at 8:14 PM Johan Hovold <johan@kernel.org> wrote: > > > > On Sat, Apr 09, 2022 at 08:09:00PM +0800, Dongliang Mu wrote: > > > From: Dongliang Mu <mudongliangabcd@gmail.com> > > > > > > cdc_ncm_bind calls cdc_ncm_bind_common and sets dev->data[0] > > > with ctx. However, in the unbind function - cdc_ncm_unbind, > > > it calls cdc_ncm_free and frees ctx, leaving dev->data[0] as > > > a dangling pointer. The following ioctl operation will trigger > > > the UAF in the function cdc_ncm_set_dgram_size. > > > > > > Fix this by setting dev->data[0] as zero. > > > > This sounds like a poor band-aid. Please explain how this prevent the > > ioctl() from racing with unbind(). > > You mean the following thread interlaving? > > ioctl unbind > cdc_ncm_free(ctx); > dev->data[0] > dev->data[0] = 0; > > It seems this will still trigger UAF. Maybe we need to add mutex to > prevent this. But I am not sure. ioctl unbind cdc_ncm_free(ctx); dev->data[0] = 0; dev->data[0] This will trigger a null pointer dereference if my patch is applied, right? > > > > > Johan > > > > > ================================================================== > > > BUG: KASAN: use-after-free in cdc_ncm_set_dgram_size+0xc91/0xde0 > > > Read of size 8 at addr ffff8880755210b0 by task dhcpcd/3174 > > > > > > Call Trace: > > > <TASK> > > > __dump_stack lib/dump_stack.c:88 [inline] > > > dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106 > > > print_address_description.constprop.0.cold+0xeb/0x495 mm/kasan/report.c:313 > > > print_report mm/kasan/report.c:429 [inline] > > > kasan_report.cold+0xf4/0x1c6 mm/kasan/report.c:491 > > > cdc_ncm_set_dgram_size+0xc91/0xde0 drivers/net/usb/cdc_ncm.c:608 > > > cdc_ncm_change_mtu+0x10c/0x140 drivers/net/usb/cdc_ncm.c:798 > > > __dev_set_mtu net/core/dev.c:8519 [inline] > > > dev_set_mtu_ext+0x352/0x5b0 net/core/dev.c:8572 > > > dev_set_mtu+0x8e/0x120 net/core/dev.c:8596 > > > dev_ifsioc+0xb87/0x1090 net/core/dev_ioctl.c:332 > > > dev_ioctl+0x1b9/0xe30 net/core/dev_ioctl.c:586 > > > sock_do_ioctl+0x15a/0x230 net/socket.c:1136 > > > sock_ioctl+0x2f1/0x640 net/socket.c:1239 > > > vfs_ioctl fs/ioctl.c:51 [inline] > > > __do_sys_ioctl fs/ioctl.c:870 [inline] > > > __se_sys_ioctl fs/ioctl.c:856 [inline] > > > __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:856 > > > do_syscall_x64 arch/x86/entry/common.c:50 [inline] > > > do_syscall_64+0x35/0x80 arch/x86/entry/common.c:80 > > > entry_SYSCALL_64_after_hwframe+0x44/0xae > > > RIP: 0033:0x7f00859e70e7 > > > RSP: 002b:00007ffedd503dd8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 > > > RAX: ffffffffffffffda RBX: 00007f00858f96c8 RCX: 00007f00859e70e7 > > > RDX: 00007ffedd513fc8 RSI: 0000000000008922 RDI: 0000000000000018 > > > RBP: 00007ffedd524178 R08: 00007ffedd513f88 R09: 00007ffedd513f38 > > > R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000 > > > R13: 00007ffedd513fc8 R14: 0000000000000028 R15: 0000000000008922 > > > </TASK> > > > > > Reported-by: syzbot+eabbf2aaa999cc507108@syzkaller.appspotmail.com > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > > --- > > > drivers/net/usb/cdc_ncm.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c > > > index 15f91d691bba..9fc2df9f0b63 100644 > > > --- a/drivers/net/usb/cdc_ncm.c > > > +++ b/drivers/net/usb/cdc_ncm.c > > > @@ -1019,6 +1019,7 @@ void cdc_ncm_unbind(struct usbnet *dev, struct usb_interface *intf) > > > > > > usb_set_intfdata(intf, NULL); > > > cdc_ncm_free(ctx); > > > + dev->data[0] = 0; > > > } > > > EXPORT_SYMBOL_GPL(cdc_ncm_unbind);
On Mon, Apr 11, 2022 at 9:33 PM Johan Hovold <johan@kernel.org> wrote: > On Sat, Apr 09, 2022 at 08:09:00PM +0800, Dongliang Mu wrote: > > From: Dongliang Mu <mudongliangabcd@gmail.com> > > > > cdc_ncm_bind calls cdc_ncm_bind_common and sets dev->data[0] > > with ctx. However, in the unbind function - cdc_ncm_unbind, > > it calls cdc_ncm_free and frees ctx, leaving dev->data[0] as > > a dangling pointer. The following ioctl operation will trigger > > the UAF in the function cdc_ncm_set_dgram_size. > > > > Fix this by setting dev->data[0] as zero. > > This sounds like a poor band-aid. Please explain how this prevent the > ioctl() from racing with unbind(). Good question. Isn't it the commit 2c9d6c2b871d ("usbnet: run unbind() before unregister_netdev()") which changed the ordering of the interface shutdown and basically makes this race happen? I don't see how we can guarantee that IOCTL won't be called until we quiescence the network device — my understanding that on device surprise removal we have to first shutdown what it created and then unbind the device. If I understand the original issue correctly then the problem is in usbnet->unbind and it should actually be split to two hooks, otherwise it seems every possible IOCTL callback must have some kind of reference counting and keep an eye on the surprise removal. Johan, can you correct me if my understanding is wrong?
On Thu, Apr 14, 2022 at 06:01:57PM +0300, Andy Shevchenko wrote: > On Mon, Apr 11, 2022 at 9:33 PM Johan Hovold <johan@kernel.org> wrote: > > On Sat, Apr 09, 2022 at 08:09:00PM +0800, Dongliang Mu wrote: > > > From: Dongliang Mu <mudongliangabcd@gmail.com> > > > > > > cdc_ncm_bind calls cdc_ncm_bind_common and sets dev->data[0] > > > with ctx. However, in the unbind function - cdc_ncm_unbind, > > > it calls cdc_ncm_free and frees ctx, leaving dev->data[0] as > > > a dangling pointer. The following ioctl operation will trigger > > > the UAF in the function cdc_ncm_set_dgram_size. > > > > > > Fix this by setting dev->data[0] as zero. > > > > This sounds like a poor band-aid. Please explain how this prevent the > > ioctl() from racing with unbind(). > > Good question. Isn't it the commit 2c9d6c2b871d ("usbnet: run unbind() > before unregister_netdev()") which changed the ordering of the > interface shutdown and basically makes this race happen? I don't see > how we can guarantee that IOCTL won't be called until we quiescence > the network device — my understanding that on device surprise removal > we have to first shutdown what it created and then unbind the device. > If I understand the original issue correctly then the problem is in > usbnet->unbind and it should actually be split to two hooks, otherwise > it seems every possible IOCTL callback must have some kind of > reference counting and keep an eye on the surprise removal. > > Johan, can you correct me if my understanding is wrong? Hi, the possible fix for this issue is under discussion here: https://lore.kernel.org/netdev/d13e3a34-7e85-92dd-d0c0-5efb3fb08182@suse.com/T/ Regards, Oleksij
On 14.04.22 17:01, Andy Shevchenko wrote: > > Good question. Isn't it the commit 2c9d6c2b871d ("usbnet: run unbind() > before unregister_netdev()") which changed the ordering of the > interface shutdown and basically makes this race happen? I don't see > how we can guarantee that IOCTL won't be called until we quiescence > the network device — my understanding that on device surprise removal True. The best we could do is introduce a mutex for ioctl() and disconnect(). That seems the least preferable solution to me. > we have to first shutdown what it created and then unbind the device. > If I understand the original issue correctly then the problem is in > usbnet->unbind and it should actually be split to two hooks, otherwise > it seems every possible IOCTL callback must have some kind of > reference counting and keep an eye on the surprise removal. > > Johan, can you correct me if my understanding is wrong? > It seems to me that fundamentally the order of actions to handle a hotunplug must mirror the order in a hotplug. We can add more hooks if that turns out to be necessary for some drivers, but the basic reverse mirrored order must be supported and I very much favor restoring it as default. So I am afraid I have to ask again, whether anybody sees a fundamental issue with the attached patch, as opposed to it not being an elegant solution? It looks to me that we are in a fundamental disagreement on the correct order in this question and there is no productive way forward other than offering both ways. Regards Oliver
Oliver Neukum <oneukum@suse.com> writes: > It seems to me that fundamentally the order of actions to handle > a hotunplug must mirror the order in a hotplug. We can add more hooks > if that turns out to be necessary for some drivers, but the basic > reverse mirrored order must be supported and I very much favor > restoring it as default. FWIW, I agree 100% with this. Please go ahead with the revert of commit 2c9d6c2b871d ("usbnet: run unbind() before unregister_netdev()"). AFAICS, your proposed new hook should solve the original problem just fine. Bjørn
On Tue, Apr 19, 2022 at 01:47:40PM +0200, Oliver Neukum wrote: > On 14.04.22 17:01, Andy Shevchenko wrote: > > > > Good question. Isn't it the commit 2c9d6c2b871d ("usbnet: run unbind() > > before unregister_netdev()") which changed the ordering of the > > interface shutdown and basically makes this race happen? I don't see > > how we can guarantee that IOCTL won't be called until we quiescence > > the network device — my understanding that on device surprise removal > True. The best we could do is introduce a mutex for ioctl() and > disconnect(). That seems the least preferable solution to me. > > we have to first shutdown what it created and then unbind the device. Yes, indeed, commit 2c9d6c2b871d ("usbnet: run unbind() before unregister_netdev()") is fundamentally broken. You can't just start freeing driver private data before deregistering the device. > > If I understand the original issue correctly then the problem is in > > usbnet->unbind and it should actually be split to two hooks, otherwise > > it seems every possible IOCTL callback must have some kind of > > reference counting and keep an eye on the surprise removal. > > > > Johan, can you correct me if my understanding is wrong? That sounds correct. I only noticed that the proposed patch looked insufficient at best and didn't really look into the backstory until just now. > It seems to me that fundamentally the order of actions to handle > a hotunplug must mirror the order in a hotplug. We can add more hooks > if that turns out to be necessary for some drivers, but the basic > reverse mirrored order must be supported and I very much favor > restoring it as default. I agree, we need to strive to maintain symmetry. Anything else is likely broken and at least makes things harder to reason about and maintenance a pain. > So I am afraid I have to ask again, whether anybody sees a fundamental > issue with the attached patch, as opposed to it not being an elegant > solution? > It looks to me that we are in a fundamental disagreement on the correct > order in this question and there is no productive way forward other than > offering both ways. > > Regards > Oliver > From 2e07ccbd1769889963d129ec474909bdcaa4c64a Mon Sep 17 00:00:00 2001 > From: Oliver Neukum <oneukum@suse.com> > Date: Thu, 10 Mar 2022 13:18:38 +0100 > Subject: [PATCH] usbnet: split unbind callback > > Some devices need to be informed of a disconnect before > the generic layer is informed, others need their notification > later to avoid race conditions. Hence we provide two callbacks. > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > --- > drivers/net/usb/asix_devices.c | 8 ++++---- > drivers/net/usb/smsc95xx.c | 4 ++-- > drivers/net/usb/usbnet.c | 7 +++++-- > include/linux/usb/usbnet.h | 3 +++ > 4 files changed, 14 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c > index 6ea44e53713a..e6cfa9a39a87 100644 > --- a/drivers/net/usb/asix_devices.c > +++ b/drivers/net/usb/asix_devices.c > @@ -808,7 +808,7 @@ static int ax88772_stop(struct usbnet *dev) > return 0; > } > > -static void ax88772_unbind(struct usbnet *dev, struct usb_interface *intf) > +static void ax88772_disable(struct usbnet *dev, struct usb_interface *intf) > { > struct asix_common_private *priv = dev->driver_priv; > > @@ -1214,7 +1214,7 @@ static const struct driver_info hawking_uf200_info = { > static const struct driver_info ax88772_info = { > .description = "ASIX AX88772 USB 2.0 Ethernet", > .bind = ax88772_bind, > - .unbind = ax88772_unbind, > + .unbind = ax88772_disable, These should all be .disable = ... but you probably need to split the callback and keep unbind as well for the actual clean up (freeing resources etc). > .status = asix_status, > .reset = ax88772_reset, > .stop = ax88772_stop, > @@ -1226,7 +1226,7 @@ static const struct driver_info ax88772_info = { > static const struct driver_info ax88772b_info = { > .description = "ASIX AX88772B USB 2.0 Ethernet", > .bind = ax88772_bind, > - .unbind = ax88772_unbind, > + .unbind = ax88772_disable, > .status = asix_status, > .reset = ax88772_reset, > .stop = ax88772_stop, > @@ -1262,7 +1262,7 @@ static const struct driver_info ax88178_info = { > static const struct driver_info hg20f9_info = { > .description = "HG20F9 USB 2.0 Ethernet", > .bind = ax88772_bind, > - .unbind = ax88772_unbind, > + .unbind = ax88772_disable, > .status = asix_status, > .reset = ax88772_reset, > .flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_LINK_INTR | > diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c > index 5567220e9d16..62db57021f5f 100644 > --- a/drivers/net/usb/smsc95xx.c > +++ b/drivers/net/usb/smsc95xx.c > @@ -1211,7 +1211,7 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf) > return ret; > } > > -static void smsc95xx_unbind(struct usbnet *dev, struct usb_interface *intf) > +static void smsc95xx_disable(struct usbnet *dev, struct usb_interface *intf) > { > struct smsc95xx_priv *pdata = dev->driver_priv; > > @@ -1985,7 +1985,7 @@ static int smsc95xx_manage_power(struct usbnet *dev, int on) > static const struct driver_info smsc95xx_info = { > .description = "smsc95xx USB 2.0 Ethernet", > .bind = smsc95xx_bind, > - .unbind = smsc95xx_unbind, > + .unbind = smsc95xx_disable, > .link_reset = smsc95xx_link_reset, > .reset = smsc95xx_reset, > .check_connect = smsc95xx_start_phy, > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > index b1f93810a6f3..5249a7d7efa5 100644 > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -1641,8 +1641,8 @@ void usbnet_disconnect (struct usb_interface *intf) > xdev->bus->bus_name, xdev->devpath, > dev->driver_info->description); > > - if (dev->driver_info->unbind) > - dev->driver_info->unbind(dev, intf); > + if (dev->driver_info->disable) > + dev->driver_info->disable(dev, intf); > > net = dev->net; > unregister_netdev (net); > @@ -1651,6 +1651,9 @@ void usbnet_disconnect (struct usb_interface *intf) > > usb_scuttle_anchored_urbs(&dev->deferred); > > + if (dev->driver_info->unbind) > + dev->driver_info->unbind (dev, intf); > + > usb_kill_urb(dev->interrupt); Don't you need to quiesce all I/O, including stopping the interrupt URB, before unbind? > usb_free_urb(dev->interrupt); > kfree(dev->padding_pkt); > diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h > index 8336e86ce606..4d2407f1ae93 100644 > --- a/include/linux/usb/usbnet.h > +++ b/include/linux/usb/usbnet.h > @@ -129,6 +129,9 @@ struct driver_info { > /* cleanup device ... can sleep, but can't fail */ > void (*unbind)(struct usbnet *, struct usb_interface *); > > + /* disable device ... can sleep, but can't fail */ > + void (*disable)(struct usbnet *, struct usb_interface *); > + > /* reset device ... can sleep */ > int (*reset)(struct usbnet *); Johan
On 20.04.22 08:56, Johan Hovold wrote: > >> >> @@ -1214,7 +1214,7 @@ static const struct driver_info hawking_uf200_info = { >> static const struct driver_info ax88772_info = { >> .description = "ASIX AX88772 USB 2.0 Ethernet", >> .bind = ax88772_bind, >> - .unbind = ax88772_unbind, >> + .unbind = ax88772_disable, > These should all be > > .disable = ... Thanks, noted. > > but you probably need to split the callback and keep unbind as well for > the actual clean up (freeing resources etc). That is the driver the problematic commit was requested for. I am looking into it. > >> - if (dev->driver_info->unbind) >> - dev->driver_info->unbind(dev, intf); >> + if (dev->driver_info->disable) >> + dev->driver_info->disable(dev, intf); >> >> net = dev->net; >> unregister_netdev (net); >> @@ -1651,6 +1651,9 @@ void usbnet_disconnect (struct usb_interface *intf) >> >> usb_scuttle_anchored_urbs(&dev->deferred); >> >> + if (dev->driver_info->unbind) >> + dev->driver_info->unbind (dev, intf); >> + >> usb_kill_urb(dev->interrupt); > Don't you need to quiesce all I/O, including stopping the interrupt URB, > before unbind? If I do that, how do I prevent people from relaunching the URB between kill and unbind? Do I need to poison it? Regards Oliver
On Wed, Apr 20, 2022 at 11:45:49AM +0200, Oliver Neukum wrote: > >> - if (dev->driver_info->unbind) > >> - dev->driver_info->unbind(dev, intf); > >> + if (dev->driver_info->disable) > >> + dev->driver_info->disable(dev, intf); > >> > >> net = dev->net; > >> unregister_netdev (net); > >> @@ -1651,6 +1651,9 @@ void usbnet_disconnect (struct usb_interface *intf) > >> > >> usb_scuttle_anchored_urbs(&dev->deferred); > >> > >> + if (dev->driver_info->unbind) > >> + dev->driver_info->unbind (dev, intf); > >> + > >> usb_kill_urb(dev->interrupt); > > Don't you need to quiesce all I/O, including stopping the interrupt URB, > > before unbind? > If I do that, how do I prevent people from relaunching the URB between > kill and unbind? Do I need to poison it? You could, but it would seem you have bigger problems if something can submit the URB after having deregistered the netdev. Looks like the URB should already have been stopped by usbnet_status_stop() so that the usb_kill_urb() above is (or should be) a noop. Johan
So I am afraid I have to ask again, whether anybody sees a fundamental issue with the new version attached patch, as opposed to it not being an elegant solution? I corrected the stuff Johan found and split the method in the asix driver. I do not understand smsc95xx well, so I left it in the current state. Regards Oliver
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c index 15f91d691bba..9fc2df9f0b63 100644 --- a/drivers/net/usb/cdc_ncm.c +++ b/drivers/net/usb/cdc_ncm.c @@ -1019,6 +1019,7 @@ void cdc_ncm_unbind(struct usbnet *dev, struct usb_interface *intf) usb_set_intfdata(intf, NULL); cdc_ncm_free(ctx); + dev->data[0] = 0; } EXPORT_SYMBOL_GPL(cdc_ncm_unbind);