Re: Several races in "usbnet" module (kernel 4.1.x)

[Eugene Shatokhin <eugene.shatokhin@rosalab.ru>]

21.07.2015 17:22, Oliver Neukum пишет:
> On Mon, 2015-07-20 at 21:13 +0300, Eugene Shatokhin wrote:
>> And here, the code clears EVENT_RX_KILL bit in dev->flags, which may
>> execute concurrently with the above operation:
>> #0 clear_bit (bitops.h:113, inlined)
>> #1 usbnet_bh (usbnet.c:1475)
>>          /* restart RX again after disabling due to high error rate */
>>          clear_bit(EVENT_RX_KILL, &dev->flags);
>>
>> If clear_bit() is atomic w.r.t. setting dev->flags to 0, this race is
>> not a problem, I guess. Otherwise, it may be.
>
> clear_bit is atomic with respect to other atomic operations.
> So how about this:
>
> 	Regards
> 		Oliver
>

Thanks for the quick replies!
My comments are below.

>>From 1c4e685b3a9c183e04c46b661830e5c7ed35b513 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oneukum@suse.com>
> Date: Tue, 21 Jul 2015 16:19:40 +0200
> Subject: [PATCH] usbnet: fix race between usbnet_stop() and the BH
>
> Does this do the job?
>
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>   drivers/net/usb/usbnet.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 3c86b10..77a9a86 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -778,7 +778,7 @@ int usbnet_stop (struct net_device *net)
>   {
>   	struct usbnet		*dev = netdev_priv(net);
>   	struct driver_info	*info = dev->driver_info;
> -	int			retval, pm;
> +	int			retval, pm, mpn;
>
>   	clear_bit(EVENT_DEV_OPEN, &dev->flags);
>   	netif_stop_queue (net);
> @@ -813,14 +813,17 @@ int usbnet_stop (struct net_device *net)
>   	 * can't flush_scheduled_work() until we drop rtnl (later),
>   	 * else workers could deadlock; so make workers a NOP.
>   	 */

> +	mpn = !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags);

Right, I missed that. Indeed, if one needs EVENT_NO_RUNTIME_PM bit, one 
should get it before dev->flags is set to 0.

>   	dev->flags = 0;

I suppose usbnet_bh() cannot be re-scheduled at this point. And if it is 
running now, tasklet_kill will wait till it finishes. So, I guess, it 
would be enough to zero dev->flags after "tasklet_kill (&dev->bh);" 
rather than before it, like it is now.

Anyway, if it is needed to clear any particular flags to prevent 
re-scheduling of usbnet_bh(), this can be done here with clear_bit(). 
Not sure if there are such flags, I am by no means an expert in usbnet.

>   	del_timer_sync (&dev->delay);
>   	tasklet_kill (&dev->bh);

The following part is not necessary, I think. usbnet_bh() does not touch 
EVENT_NO_RUNTIME_PM bit explicitly and these bit operations are atomic 
w.r.t. each other.

> +	mpn |= !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags);
> +	/* in case the bh reset a flag */

But zeroing dev->flags here is necessary, I agree.

> +	dev->flags = 0;

>   	if (!pm)
>   		usb_autopm_put_interface(dev->intf);
>
> -	if (info->manage_power &&
> -	    !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags))
> +	if (info->manage_power && mpn)
>   		info->manage_power(dev, 0);
>   	else
>   		usb_autopm_put_interface(dev->intf);
>

Regards,
Eugene