| Message ID | 20231121144330.3990-1-oneukum@suse.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC] USB: ipeth: race between ipeth_close and error handling | expand |
Hello Oliver, Thank you for the patch and patience! > ipheth_sndbulk_callback() can submit carrier_work > as a part of its error handling. That means that > the driver must make sure that the work is cancelled > after it has made sure that no more URB can terminate > with an error condition. > > Hence the order of actions in ipeth_close() needs > to be inverted. The change looks reasonable to me. It's been a while, but do you perhaps recall how you stumbled upon this? Was that a proactive fix, or was it in response to an issue you (or someone else) encountered? Basically wondering if this is something I could test/reproduce. I'm planning to submit a few patches for ipheth shortly, would it be alright with you if I included yours in the series as well? If so, I'll fix ipeth -> ipheth spelling if you don't mind. Cheers, Foster
On 01.08.24 21:42, Foster Snowhill wrote: > Hello Oliver, > > Thank you for the patch and patience! > >> ipheth_sndbulk_callback() can submit carrier_work >> as a part of its error handling. That means that >> the driver must make sure that the work is cancelled >> after it has made sure that no more URB can terminate >> with an error condition. >> >> Hence the order of actions in ipeth_close() needs >> to be inverted. > > The change looks reasonable to me. It's been a while, but do you perhaps > recall how you stumbled upon this? Was that a proactive fix, or was it > in response to an issue you (or someone else) encountered? Basically > wondering if this is something I could test/reproduce. Hi, sorry I was on vacation. I think I looked at the driver because of an unrelated patch and saw this issue. That bug type is not exactly uncommon. HTH Oliver
diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c index 687d70cfc556..6eeef10edada 100644 --- a/drivers/net/usb/ipheth.c +++ b/drivers/net/usb/ipheth.c @@ -475,8 +475,8 @@ static int ipheth_close(struct net_device *net) { struct ipheth_device *dev = netdev_priv(net); - cancel_delayed_work_sync(&dev->carrier_work); netif_stop_queue(net); + cancel_delayed_work_sync(&dev->carrier_work); return 0; }
ipheth_sndbulk_callback() can submit carrier_work as a part of its error handling. That means that the driver must make sure that the work is cancelled after it has made sure that no more URB can terminate with an error condition. Hence the order of actions in ipeth_close() needs to be inverted. Signed-off-by: Oliver Neukum <oneukum@suse.com> --- drivers/net/usb/ipheth.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)