RE: [PATCH net-next] usbnet: fix cyclical race on disconnect with work queue
From: Sai Krishna Gajula
Date: Fri Mar 22 2024 - 13:43:51 EST
> -----Original Message-----
> From: Oliver Neukum <oneukum@xxxxxxxx>
> Sent: Thursday, March 21, 2024 6:17 PM
> To: davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Cc: Oliver Neukum <oneukum@xxxxxxxx>;
> syzbot+9665bf55b1c828bbcd8a@xxxxxxxxxxxxxxxxxxxxxxxxx
> Subject: [PATCH net-next] usbnet: fix cyclical race on disconnect
> with work queue
This patch seems to be a fix, in that case the subject need to be with [PATCH net]
>
> The work can submit URBs and the URBs can schedule the work.
> This cycle needs to be broken, when a device is to be stopped.
> Use a flag to do so.
>
> Fixes: f29fc259976e9 ("[PATCH] USB: usbnet (1/9) clean up framing")
Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: f29fc259976e ("[PATCH] USB: usbnet (1/9) clean up framing")'
> Reported-by: syzbot+9665bf55b1c828bbcd8a@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Oliver Neukum <oneukum@xxxxxxxx>
> ---
> drivers/net/usb/usbnet.c | 37 ++++++++++++++++++++++++++++---------
> include/linux/usb/usbnet.h | 18 ++++++++++++++++++
> 2 files changed, 46 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index
> e84efa661589..422d91635045 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -467,10 +467,12 @@ static enum skb_state defer_bh(struct usbnet *dev,
> struct sk_buff *skb, void usbnet_defer_kevent (struct usbnet *dev, int work)
space prohibited between function name and open parenthesis '('
> {
> set_bit (work, &dev->flags);
> - if (!schedule_work (&dev->kevent))
> - netdev_dbg(dev->net, "kevent %s may have been
> dropped\n", usbnet_event_names[work]);
> - else
> - netdev_dbg(dev->net, "kevent %s scheduled\n",
> usbnet_event_names[work]);
> + if (!usbnet_going_away(dev)) {
> + if (!schedule_work (&dev->kevent))
> + netdev_dbg(dev->net, "kevent %s may have been
> dropped\n", usbnet_event_names[work]);
> + else
> + netdev_dbg(dev->net, "kevent %s scheduled\n",
> usbnet_event_names[work]);
> + }
> }
> EXPORT_SYMBOL_GPL(usbnet_defer_kevent);
>
> @@ -538,7 +540,8 @@ static int rx_submit (struct usbnet *dev, struct urb
> *urb, gfp_t flags)
space prohibited between function name and open parenthesis '(', where ever applicable.
> tasklet_schedule (&dev->bh);
> break;
> case 0:
> - __usbnet_queue_skb(&dev->rxq, skb, rx_start);
> + if (!usbnet_going_away(dev))
> + __usbnet_queue_skb(&dev->rxq, skb,
> rx_start);
> }
> } else {
> netif_dbg(dev, ifdown, dev->net, "rx: stopped\n"); @@ -849,6
> +852,16 @@ int usbnet_stop (struct net_device *net)
> del_timer_sync (&dev->delay);
> tasklet_kill (&dev->bh);
> cancel_work_sync(&dev->kevent);
> +
> + /*
> + * we have cyclic dependencies. Those calls are needed
> + * to break a cycle. We cannot fall into the gaps because
> + * we have a flag
> + */
Networking block comments don't use an empty /* line, use /* Comment...
> + tasklet_kill (&dev->bh);
> + del_timer_sync (&dev->delay);
> + cancel_work_sync(&dev->kevent);
> +
> if (!pm)
> usb_autopm_put_interface(dev->intf);
>
> @@ -1174,7 +1187,8 @@ usbnet_deferred_kevent (struct work_struct *work)
> status);
> } else {
> clear_bit (EVENT_RX_HALT, &dev->flags);
> - tasklet_schedule (&dev->bh);
> + if (!usbnet_going_away(dev))
> + tasklet_schedule (&dev->bh);
> }
> }
>
> @@ -1196,10 +1210,13 @@ usbnet_deferred_kevent (struct work_struct
> *work)
> }
> if (rx_submit (dev, urb, GFP_KERNEL) == -ENOLINK)
> resched = 0;
> - usb_autopm_put_interface(dev->intf);
> fail_lowmem:
> - if (resched)
> + usb_autopm_put_interface(dev->intf);
> + if (resched) {
> + set_bit (EVENT_RX_MEMORY, &dev->flags);
> +
> tasklet_schedule (&dev->bh);
> + }
> }
> }
>
> @@ -1212,13 +1229,13 @@ usbnet_deferred_kevent (struct work_struct
> *work)
> if (status < 0)
> goto skip_reset;
> if(info->link_reset && (retval = info->link_reset(dev)) < 0) {
> - usb_autopm_put_interface(dev->intf);
> skip_reset:
> netdev_info(dev->net, "link reset failed (%d) usbnet
> usb-%s-%s, %s\n",
> retval,
> dev->udev->bus->bus_name,
> dev->udev->devpath,
> info->description);
> + usb_autopm_put_interface(dev->intf);
> } else {
> usb_autopm_put_interface(dev->intf);
> }
> @@ -1562,6 +1579,7 @@ static void usbnet_bh (struct timer_list *t)
> } else if (netif_running (dev->net) &&
> netif_device_present (dev->net) &&
> netif_carrier_ok(dev->net) &&
> + !usbnet_going_away(dev) &&
> !timer_pending(&dev->delay) &&
> !test_bit(EVENT_RX_PAUSED, &dev->flags) &&
> !test_bit(EVENT_RX_HALT, &dev->flags)) { @@ -1609,6
> +1627,7 @@ void usbnet_disconnect (struct usb_interface *intf)
> usb_set_intfdata(intf, NULL);
> if (!dev)
> return;
> + usbnet_mark_going_away(dev);
>
> xdev = interface_to_usbdev (intf);
>
> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h index
> 9f08a584d707..d26599faab33 100644
> --- a/include/linux/usb/usbnet.h
> +++ b/include/linux/usb/usbnet.h
> @@ -76,8 +76,26 @@ struct usbnet {
> # define EVENT_LINK_CHANGE 11
> # define EVENT_SET_RX_MODE 12
> # define EVENT_NO_IP_ALIGN 13
> +/*
> + * this one is special, as it indicates that the device is going away
> + * there are cyclic dependencies between tasklet, timer and bh
> + * that must be broken
> + */
Networking block comments don't use an empty /* line, use /* Comment...
> +# define EVENT_UNPLUG 31
> };
>
> +static inline bool usbnet_going_away(struct usbnet *ubn) {
> + smp_mb__before_atomic();
> + return test_bit(EVENT_UNPLUG, &ubn->flags); }
> +
> +static inline void usbnet_mark_going_away(struct usbnet *ubn) {
> + set_bit(EVENT_UNPLUG, &ubn->flags);
> + smp_mb__after_atomic();
> +}
> +
> static inline struct usb_driver *driver_of(struct usb_interface *intf) {
> return to_usb_driver(intf->dev.driver);
> --
> 2.44.0
>