[RFC] USB: ipeth: race between ipeth_close and error handling

[RFC] USB: ipeth: race between ipeth_close and error handling

[Oliver Neukum]

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(-)

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;
 }
 
-- 
2.42.1

Re: [RFC] USB: ipeth: race between ipeth_close and error handling

[Foster Snowhill]

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

Re: [RFC] USB: ipeth: race between ipeth_close and error handling

[Oliver Neukum]

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