| Message ID | 20240321124758.6302-1-oneukum@suse.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] usbnet: fix cyclical race on disconnect with work queue | expand |
On Thu, Mar 21, 2024 at 01:46:41PM +0100, Oliver Neukum wrote: > 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") > Reported-by: syzbot+9665bf55b1c828bbcd8a@syzkaller.appspotmail.com > Signed-off-by: Oliver Neukum <oneukum@suse.com> > --- > drivers/net/usb/usbnet.c | 37 ++++++++++++++++++++++++++++--------- > include/linux/usb/usbnet.h | 18 ++++++++++++++++++ > 2 files changed, 46 insertions(+), 9 deletions(-) > Hi, This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree. You are receiving this message because of the following common error(s) as indicated below: - You have marked a patch with a "Fixes:" tag for a commit that is in an older released kernel, yet you do not have a cc: stable line in the signed-off-by area at all, which means that the patch will not be applied to any older kernel releases. To properly fix this, please follow the documented rules in the Documentation/process/stable-kernel-rules.rst file for how to resolve this. If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers. thanks, greg k-h's patch email bot
> -----Original Message----- > From: Oliver Neukum <oneukum@suse.com> > Sent: Thursday, March 21, 2024 6:17 PM > To: davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; netdev@vger.kernel.org; linux-usb@vger.kernel.org; > linux-kernel@vger.kernel.org > Cc: Oliver Neukum <oneukum@suse.com>; > syzbot+9665bf55b1c828bbcd8a@syzkaller.appspotmail.com > 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@syzkaller.appspotmail.com > Signed-off-by: Oliver Neukum <oneukum@suse.com> > --- > 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 >
On 3/22/24 18:43, Sai Krishna Gajula wrote: > >> -----Original Message----- >> From: Oliver Neukum <oneukum@suse.com> >> Sent: Thursday, March 21, 2024 6:17 PM >> To: davem@davemloft.net; edumazet@google.com; kuba@kernel.org; >> pabeni@redhat.com; netdev@vger.kernel.org; linux-usb@vger.kernel.org; >> linux-kernel@vger.kernel.org >> Cc: Oliver Neukum <oneukum@suse.com>; >> syzbot+9665bf55b1c828bbcd8a@syzkaller.appspotmail.com >> 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] OK > >> >> 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")' Ehm, what exactly did I do differently >> --- 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 '(' I am sorry, but this is the context of the diff. You are not suggesting to mix gratitious format changes into a bug fix, are you? >> 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... OK Regards Oliver
On Wed, Mar 27, 2024 at 04:10:36PM +0100, Oliver Neukum wrote: > > > On 3/22/24 18:43, Sai Krishna Gajula wrote: > > > > > -----Original Message----- > > > From: Oliver Neukum <oneukum@suse.com> > > > Sent: Thursday, March 21, 2024 6:17 PM > > > To: davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > > > pabeni@redhat.com; netdev@vger.kernel.org; linux-usb@vger.kernel.org; > > > linux-kernel@vger.kernel.org > > > Cc: Oliver Neukum <oneukum@suse.com>; > > > syzbot+9665bf55b1c828bbcd8a@syzkaller.appspotmail.com > > > 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] > > OK > > > > > > > > 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")' > > Ehm, what exactly did I do differently I think the point being made is that the hash has 13 rather than 12 characters. But, IMHO, that is fine because my understanding is that the requirement is that the hash is at least, not exactly, 12 characters long. ...
> -----Original Message----- > From: Oliver Neukum <oneukum@suse.com> > Sent: Wednesday, March 27, 2024 8:41 PM > To: Sai Krishna Gajula <saikrishnag@marvell.com>; Oliver Neukum > <oneukum@suse.com>; davem@davemloft.net; edumazet@google.com; > kuba@kernel.org; pabeni@redhat.com; netdev@vger.kernel.org; linux- > usb@vger.kernel.org; linux-kernel@vger.kernel.org > Cc: syzbot+9665bf55b1c828bbcd8a@syzkaller.appspotmail.com > Subject: Re: [PATCH net-next] usbnet: fix cyclical race on > disconnect with work queue > > > On 3/22/24 18:43, Sai Krishna Gajula wrote: > > > >> -----Original Message----- > >> From: Oliver Neukum <oneukum@suse.com> > >> Sent: Thursday, March 21, 2024 6:17 PM > >> To: davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > >> pabeni@redhat.com; netdev@vger.kernel.org; linux-usb@vger.kernel.org; > >> linux-kernel@vger.kernel.org > >> Cc: Oliver Neukum <oneukum@suse.com>; > >> syzbot+9665bf55b1c828bbcd8a@syzkaller.appspotmail.com > >> 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] > > OK > > > >> > >> 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")' > > Ehm, what exactly did I do differently Checkpatch triggers warning if we give Fixes tag(f29fc259976e -9-) more than 12 chars. It is advisable to follow the upstreaming process steps to have uniformity across patches. > > >> --- 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 '(' > > I am sorry, but this is the context of the diff. You are not suggesting to mix > gratitious format changes into a bug fix, are you? Checkpatch does a primary check and triggered warning if we use space between fn name and '('. It is advisable to follow the upstreaming process steps(clean checkpatch output) to have uniformity across patches. Also check comments from Gregkh bot regarding this patch. > > >> 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... > > OK > > Regards > Oliver
On 31.03.24 14:07, Sai Krishna Gajula wrote: >>>> --- 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 '(' >> >> I am sorry, but this is the context of the diff. You are not suggesting to mix >> gratitious format changes into a bug fix, are you? > > Checkpatch does a primary check and triggered warning if we use space between fn name and '('. It is advisable to follow the upstreaming process steps(clean checkpatch output) to have uniformity across patches. Also check comments from Gregkh bot regarding this patch. Hi, at the risk of repeating myself: This is a very old driver written long before these conventions. I did not introduce these spaces. Nevertheless, a format change has no place in a bug fix. Regards Oliver
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) { 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) 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 + */ + 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 + */ +# 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);
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") Reported-by: syzbot+9665bf55b1c828bbcd8a@syzkaller.appspotmail.com Signed-off-by: Oliver Neukum <oneukum@suse.com> --- drivers/net/usb/usbnet.c | 37 ++++++++++++++++++++++++++++--------- include/linux/usb/usbnet.h | 18 ++++++++++++++++++ 2 files changed, 46 insertions(+), 9 deletions(-)