* Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free
2022-04-14 15:01 ` Andy Shevchenko
@ 2022-04-15 7:19 ` Oleksij Rempel
2022-04-19 11:47 ` Oliver Neukum
2022-04-21 11:18 ` Oliver Neukum
2 siblings, 0 replies; 14+ messages in thread
From: Oleksij Rempel @ 2022-04-15 7:19 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Johan Hovold, Oleksij Rempel, Dongliang Mu, Oliver Neukum,
David S. Miller, Jakub Kicinski, Paolo Abeni, Dongliang Mu,
syzbot+eabbf2aaa999cc507108, USB, netdev,
Linux Kernel Mailing List
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
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free
2022-04-14 15:01 ` Andy Shevchenko
2022-04-15 7:19 ` Oleksij Rempel
@ 2022-04-19 11:47 ` Oliver Neukum
2022-04-19 20:25 ` Bjørn Mork
2022-04-20 6:56 ` Johan Hovold
2022-04-21 11:18 ` Oliver Neukum
2 siblings, 2 replies; 14+ messages in thread
From: Oliver Neukum @ 2022-04-19 11:47 UTC (permalink / raw)
To: Andy Shevchenko, Johan Hovold, Oleksij Rempel
Cc: Dongliang Mu, Oliver Neukum, David S. Miller, Jakub Kicinski,
Paolo Abeni, Dongliang Mu, syzbot+eabbf2aaa999cc507108, USB,
netdev, Linux Kernel Mailing List
[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]
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
[-- Attachment #2: 0001-usbnet-split-unbind-callback.patch --]
[-- Type: text/x-patch, Size: 4357 bytes --]
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,
.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);
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 *);
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free
2022-04-19 11:47 ` Oliver Neukum
@ 2022-04-19 20:25 ` Bjørn Mork
2022-04-20 6:56 ` Johan Hovold
1 sibling, 0 replies; 14+ messages in thread
From: Bjørn Mork @ 2022-04-19 20:25 UTC (permalink / raw)
To: Oliver Neukum
Cc: Andy Shevchenko, Johan Hovold, Oleksij Rempel, Dongliang Mu,
Oliver Neukum, David S. Miller, Jakub Kicinski, Paolo Abeni,
Dongliang Mu, syzbot+eabbf2aaa999cc507108, USB, netdev,
Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free
2022-04-19 11:47 ` Oliver Neukum
2022-04-19 20:25 ` Bjørn Mork
@ 2022-04-20 6:56 ` Johan Hovold
2022-04-20 9:45 ` Oliver Neukum
1 sibling, 1 reply; 14+ messages in thread
From: Johan Hovold @ 2022-04-20 6:56 UTC (permalink / raw)
To: Oliver Neukum
Cc: Andy Shevchenko, Oleksij Rempel, Dongliang Mu, Oliver Neukum,
David S. Miller, Jakub Kicinski, Paolo Abeni, Dongliang Mu,
syzbot+eabbf2aaa999cc507108, USB, netdev,
Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free
2022-04-20 6:56 ` Johan Hovold
@ 2022-04-20 9:45 ` Oliver Neukum
2022-04-20 10:06 ` Johan Hovold
0 siblings, 1 reply; 14+ messages in thread
From: Oliver Neukum @ 2022-04-20 9:45 UTC (permalink / raw)
To: Johan Hovold, Oliver Neukum
Cc: Andy Shevchenko, Oleksij Rempel, Dongliang Mu, Oliver Neukum,
David S. Miller, Jakub Kicinski, Paolo Abeni, Dongliang Mu,
syzbot+eabbf2aaa999cc507108, USB, netdev,
Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free
2022-04-20 9:45 ` Oliver Neukum
@ 2022-04-20 10:06 ` Johan Hovold
0 siblings, 0 replies; 14+ messages in thread
From: Johan Hovold @ 2022-04-20 10:06 UTC (permalink / raw)
To: Oliver Neukum
Cc: Andy Shevchenko, Oleksij Rempel, Dongliang Mu, Oliver Neukum,
David S. Miller, Jakub Kicinski, Paolo Abeni, Dongliang Mu,
syzbot+eabbf2aaa999cc507108, USB, netdev,
Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free
2022-04-14 15:01 ` Andy Shevchenko
2022-04-15 7:19 ` Oleksij Rempel
2022-04-19 11:47 ` Oliver Neukum
@ 2022-04-21 11:18 ` Oliver Neukum
2 siblings, 0 replies; 14+ messages in thread
From: Oliver Neukum @ 2022-04-21 11:18 UTC (permalink / raw)
To: Andy Shevchenko, Johan Hovold, Oleksij Rempel
Cc: Dongliang Mu, Oliver Neukum, David S. Miller, Jakub Kicinski,
Paolo Abeni, Dongliang Mu, syzbot+eabbf2aaa999cc507108, USB,
netdev, Linux Kernel Mailing List, steve.glendinning
[-- Attachment #1: Type: text/plain, Size: 348 bytes --]
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
[-- Attachment #2: 0001-usbnet-split-unbind-callback.patch --]
[-- Type: text/x-patch, Size: 4554 bytes --]
From 5953b3b12dd6cdd8d0bdb0119ee627d62219ab1e 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 | 13 +++++++++++--
drivers/net/usb/smsc95xx.c | 4 ++--
drivers/net/usb/usbnet.c | 10 +++++++---
include/linux/usb/usbnet.h | 3 +++
4 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 38e47a93fb83..cde4d234b975 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -804,12 +804,18 @@ 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;
phy_disconnect(priv->phydev);
- asix_rx_fixup_common_free(dev->driver_priv);
+}
+
+static void ax88772_unbind(struct usbnet *dev, struct usb_interface *intf)
+{
+ struct asix_common_private *priv = dev->driver_priv;
+
+ asix_rx_fixup_common_free(priv);
}
static void ax88178_unbind(struct usbnet *dev, struct usb_interface *intf)
@@ -1211,6 +1217,7 @@ static const struct driver_info ax88772_info = {
.description = "ASIX AX88772 USB 2.0 Ethernet",
.bind = ax88772_bind,
.unbind = ax88772_unbind,
+ .disable = ax88772_disable,
.status = asix_status,
.reset = ax88772_reset,
.stop = ax88772_stop,
@@ -1223,6 +1230,7 @@ static const struct driver_info ax88772b_info = {
.description = "ASIX AX88772B USB 2.0 Ethernet",
.bind = ax88772_bind,
.unbind = ax88772_unbind,
+ .disable = ax88772_disable,
.status = asix_status,
.reset = ax88772_reset,
.stop = ax88772_stop,
@@ -1259,6 +1267,7 @@ static const struct driver_info hg20f9_info = {
.description = "HG20F9 USB 2.0 Ethernet",
.bind = ax88772_bind,
.unbind = ax88772_unbind,
+ .disable = 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 4ef61f6b85df..20ce88373809 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1223,7 +1223,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;
@@ -1997,7 +1997,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,
+ .disable = 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..5f3851e61573 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1641,17 +1641,21 @@ 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);
+ usb_kill_urb(dev->interrupt);
+
cancel_work_sync(&dev->kevent);
usb_scuttle_anchored_urbs(&dev->deferred);
- usb_kill_urb(dev->interrupt);
+ if (dev->driver_info->unbind)
+ dev->driver_info->unbind (dev, intf);
+
usb_free_urb(dev->interrupt);
kfree(dev->padding_pkt);
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 1b4d72d5e891..dd4a1104e332 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 *);
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread