Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1212118 > unrolled thread

Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH Eugene Shatokhin <eugene.shatokhin@rosalab.ru> - 2015-08-24 14:30 +0200
    Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH Bjørn Mork <bjorn@mork.no> - 2015-08-24 15:40 +0200
      Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH Eugene Shatokhin <eugene.shatokhin@rosalab.ru> - 2015-08-24 19:10 +0200
      Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH Oliver Neukum <oneukum@suse.com> - 2015-08-25 14:40 +0200

#1212118 — Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH

19.08.2015 15:31, Bjørn Mork пишет:
> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:
>
>> The problem is not in the reordering but rather in the fact that
>> "dev->flags = 0" is not necessarily atomic
>> w.r.t. "clear_bit(EVENT_RX_KILL, &dev->flags)", and vice versa.
>>
>> So the following might be possible, although unlikely:
>>
>> CPU0             CPU1
>>                   clear_bit: read dev->flags
>>                   clear_bit: clear EVENT_RX_KILL in the read value
>>
>> dev->flags=0;
>>
>>                   clear_bit: write updated dev->flags
>>
>> As a result, dev->flags may become non-zero again.
>
> Ah, right.  Thanks for explaining.
>
>> I cannot prove yet that this is an impossible situation. If anyone
>> can, please explain. If so, this part of the patch will not be needed.
>
> I wonder if we could simply move the dev->flags = 0 down a few lines to
> fix both issues?  It doesn't seem to do anything useful except for
> resetting the flags to a sane initial state after the device is down.
>
> Stopping the tasklet rescheduling etc depends only on netif_running(),
> which will be false when usbnet_stop is called.  There is no need to
> touch dev->flags for this to happen.

That was one of the first ideas we discussed here. Unfortunately, it is 
probably not so simple.

Setting dev->flags to 0 makes some delayed operations do nothing and, 
among other things, not to reschedule usbnet_bh().

As you can see in drivers/net/usb/usbnet.c, usbnet_bh() can be called as 
a tasklet function and as a timer function in a number of situations 
(look for the usage of dev->bh and dev->delay there).

netif_running() is indeed false when usbnet_stop() runs, usbnet_stop() 
also disables Tx. This seems to be enough for many cases where 
usbnet_bh() is scheduled, but I am not so sure about the remaining ones, 
namely:

1. A work function, usbnet_deferred_kevent(), may reschedule 
usbnet_bh(). Looks like the workqueue is only stopped in 
usbnet_disconnect(), so a work item might be processed while 
usbnet_stop() works. Setting dev->flags to 0 makes the work function do 
nothing, by the way. See also the comment in usbnet_stop() about this.

A work item may be placed to this workqueue in a number of ways, by both 
usbnet module and the mini-drivers. It is not too easy to track all 
these situations.

2. rx_complete() and tx_complete() may schedule execution of usbnet_bh() 
as a tasklet or a timer function. These two are URB completion callbacks.

It seems, new Rx and Tx URBs cannot be submitted when usbnet_stop() 
clears dev->flags, indeed. But it does not prevent the completion 
handlers for the previously submitted URBs from running concurrently 
with usbnet_stop(). The latter waits for them to complete (via 
usbnet_terminate_urbs(dev)) but only if FLAG_AVOID_UNLINK_URBS is not 
set in info->flags. rndis_wlan, however, sets this flag for a few 
hardware models. So - no guarantees here as well.

If someone could list the particular bits of dev->flags that should be 
cleared to make sure no deferred call could reschedule usbnet_bh(), 
etc... Well, it would be enough to clear these first and use dev->flags 
= 0 later, after tasklet_kill() and del_timer_sync(). I cannot point out 
these particular bits now.

Besides, it is possible, although unlikely, that new event bits will be 
added to dev->flags in the future. And one will need to keep track of 
these to see if they should be cleared as well. I'd prever to play safer 
for now and clear them all.

>
>>> The EVENT_NO_RUNTIME_PM bug should definitely be fixed.  Please split
>>> that out as a separate fix.  It's a separate issue, and should be
>>> backported to all maintained stable releases it applies to (anything
>>> from v3.8 and newer)
>>
>> Yes, that makes sense. However, this fix was originally provided by
>> Oliver Neukum rather than me, so I would like to hear his opinion as
>> well first.
>
> If what I write above is correct (please help me verify...), then maybe
> it does make sense to do these together anyway.

I think, your suggestion to make it a separate patch is right. Will do 
it in the next version of the patchset, hopefully soon.

Regards,
Eugene

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1212190

Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:

> 19.08.2015 15:31, Bjørn Mork пишет:
>> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:
>>
>>> The problem is not in the reordering but rather in the fact that
>>> "dev->flags = 0" is not necessarily atomic
>>> w.r.t. "clear_bit(EVENT_RX_KILL, &dev->flags)", and vice versa.
>>>
>>> So the following might be possible, although unlikely:
>>>
>>> CPU0             CPU1
>>>                   clear_bit: read dev->flags
>>>                   clear_bit: clear EVENT_RX_KILL in the read value
>>>
>>> dev->flags=0;
>>>
>>>                   clear_bit: write updated dev->flags
>>>
>>> As a result, dev->flags may become non-zero again.
>>
>> Ah, right.  Thanks for explaining.
>>
>>> I cannot prove yet that this is an impossible situation. If anyone
>>> can, please explain. If so, this part of the patch will not be needed.
>>
>> I wonder if we could simply move the dev->flags = 0 down a few lines to
>> fix both issues?  It doesn't seem to do anything useful except for
>> resetting the flags to a sane initial state after the device is down.
>>
>> Stopping the tasklet rescheduling etc depends only on netif_running(),
>> which will be false when usbnet_stop is called.  There is no need to
>> touch dev->flags for this to happen.
>
> That was one of the first ideas we discussed here. Unfortunately, it
> is probably not so simple.
>
> Setting dev->flags to 0 makes some delayed operations do nothing and,
> among other things, not to reschedule usbnet_bh().

Yes, but I believe that is merely a side effect.  You should never need
to clear multiple flags to get the desired behaviour.

> As you can see in drivers/net/usb/usbnet.c, usbnet_bh() can be called
> as a tasklet function and as a timer function in a number of
> situations (look for the usage of dev->bh and dev->delay there).
>
> netif_running() is indeed false when usbnet_stop() runs, usbnet_stop()
> also disables Tx. This seems to be enough for many cases where
> usbnet_bh() is scheduled, but I am not so sure about the remaining
> ones, namely:
>
> 1. A work function, usbnet_deferred_kevent(), may reschedule
> usbnet_bh(). Looks like the workqueue is only stopped in
> usbnet_disconnect(), so a work item might be processed while
> usbnet_stop() works. Setting dev->flags to 0 makes the work function
> do nothing, by the way. See also the comment in usbnet_stop() about
> this.
>
> A work item may be placed to this workqueue in a number of ways, by
> both usbnet module and the mini-drivers. It is not too easy to track
> all these situations.

That's an understatement :)



> 2. rx_complete() and tx_complete() may schedule execution of
> usbnet_bh() as a tasklet or a timer function. These two are URB
> completion callbacks.
>
> It seems, new Rx and Tx URBs cannot be submitted when usbnet_stop()
> clears dev->flags, indeed. But it does not prevent the completion
> handlers for the previously submitted URBs from running concurrently
> with usbnet_stop(). The latter waits for them to complete (via
> usbnet_terminate_urbs(dev)) but only if FLAG_AVOID_UNLINK_URBS is not
> set in info->flags. rndis_wlan, however, sets this flag for a few
> hardware models. So - no guarantees here as well.

FLAG_AVOID_UNLINK_URBS looks like it should be replaced by the newer
ability to keep the status urb active. I believe that must have been the
real reason for adding it, based on the commit message and the effect
the flag will have:

 commit 1487cd5e76337555737cbc55d7d83f41460d198f
 Author: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
 Date:   Thu Jul 30 19:41:20 2009 +0300

    usbnet: allow "minidriver" to prevent urb unlinking on usbnet_stop
    
    rndis_wlan devices freeze after running usbnet_stop several times. It appears
    that firmware freezes in state where it does not respond to any RNDIS commands
    and device have to be physically unplugged/replugged. This patch lets
    minidrivers to disable unlink_urbs on usbnet_stop through new info flag.
    
    Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
    Cc: David Brownell <dbrownell@users.sourceforge.net>
    Signed-off-by: John W. Linville <linville@tuxdriver.com>



The rx urbs will not be resubmitted in any case, and there are of course
no tx urbs being submitted.  So the only effect of this flag is on the
status/interrupt urb, which I can imagine some RNDIS devices wants
active all the time. 

So FLAG_AVOID_UNLINK_URBS should probably be removed and replaced calls
to usbnet_status_start() and usbnet_status_stop().  This will require
testing on some of the devices with the original firmware problem
however.

In any case: I do not think this flag should be considered when trying
to make usbnet_stop behaviour saner.  It's only purpose is to
deliberately break usbnet_stop by not actually stopping.


> If someone could list the particular bits of dev->flags that should be
> cleared to make sure no deferred call could reschedule usbnet_bh(),
> etc... Well, it would be enough to clear these first and use
> dev->flags = 0 later, after tasklet_kill() and del_timer_sync(). I
> cannot point out these particular bits now.


I don't think any of the flags must be cleared.  The sequence

        dev_close(dev->net);
	usbnet_terminate_urbs(dev);
	usbnet_purge_paused_rxq(dev);
	del_timer_sync (&dev->delay);
	tasklet_kill (&dev->bh);

should prevent any rescheduling of usbnet_bh

> Besides, it is possible, although unlikely, that new event bits will
> be added to dev->flags in the future. And one will need to keep track
> of these to see if they should be cleared as well. I'd prever to play
> safer for now and clear them all.

I don't think we should ever make a flag which will _have_ to be reset
for usbnet_stop.  The only reason for clearing all flags is to reset the
state before the next open.

Yes, I see that we currently need to clear EVENT_DEV_OPEN in
usbnet_stop, but I really don't see what this flag gives us which isn't
already provided by netif_running().  It looks like a duplicate.


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212363

24.08.2015 16:29, Bjørn Mork пишет:
> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:
>
>> 19.08.2015 15:31, Bjørn Mork пишет:
>>> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:
>>>
>>>> The problem is not in the reordering but rather in the fact that
>>>> "dev->flags = 0" is not necessarily atomic
>>>> w.r.t. "clear_bit(EVENT_RX_KILL, &dev->flags)", and vice versa.
>>>>
>>>> So the following might be possible, although unlikely:
>>>>
>>>> CPU0             CPU1
>>>>                    clear_bit: read dev->flags
>>>>                    clear_bit: clear EVENT_RX_KILL in the read value
>>>>
>>>> dev->flags=0;
>>>>
>>>>                    clear_bit: write updated dev->flags
>>>>
>>>> As a result, dev->flags may become non-zero again.
>>>
>>> Ah, right.  Thanks for explaining.
>>>
>>>> I cannot prove yet that this is an impossible situation. If anyone
>>>> can, please explain. If so, this part of the patch will not be needed.
>>>
>>> I wonder if we could simply move the dev->flags = 0 down a few lines to
>>> fix both issues?  It doesn't seem to do anything useful except for
>>> resetting the flags to a sane initial state after the device is down.
>>>
>>> Stopping the tasklet rescheduling etc depends only on netif_running(),
>>> which will be false when usbnet_stop is called.  There is no need to
>>> touch dev->flags for this to happen.
>>
>> That was one of the first ideas we discussed here. Unfortunately, it
>> is probably not so simple.
>>
>> Setting dev->flags to 0 makes some delayed operations do nothing and,
>> among other things, not to reschedule usbnet_bh().
>
> Yes, but I believe that is merely a side effect.  You should never need
> to clear multiple flags to get the desired behaviour.
>
>> As you can see in drivers/net/usb/usbnet.c, usbnet_bh() can be called
>> as a tasklet function and as a timer function in a number of
>> situations (look for the usage of dev->bh and dev->delay there).
>>
>> netif_running() is indeed false when usbnet_stop() runs, usbnet_stop()
>> also disables Tx. This seems to be enough for many cases where
>> usbnet_bh() is scheduled, but I am not so sure about the remaining
>> ones, namely:
>>
>> 1. A work function, usbnet_deferred_kevent(), may reschedule
>> usbnet_bh(). Looks like the workqueue is only stopped in
>> usbnet_disconnect(), so a work item might be processed while
>> usbnet_stop() works. Setting dev->flags to 0 makes the work function
>> do nothing, by the way. See also the comment in usbnet_stop() about
>> this.
>>
>> A work item may be placed to this workqueue in a number of ways, by
>> both usbnet module and the mini-drivers. It is not too easy to track
>> all these situations.
>
> That's an understatement :)
>
>
>
>> 2. rx_complete() and tx_complete() may schedule execution of
>> usbnet_bh() as a tasklet or a timer function. These two are URB
>> completion callbacks.
>>
>> It seems, new Rx and Tx URBs cannot be submitted when usbnet_stop()
>> clears dev->flags, indeed. But it does not prevent the completion
>> handlers for the previously submitted URBs from running concurrently
>> with usbnet_stop(). The latter waits for them to complete (via
>> usbnet_terminate_urbs(dev)) but only if FLAG_AVOID_UNLINK_URBS is not
>> set in info->flags. rndis_wlan, however, sets this flag for a few
>> hardware models. So - no guarantees here as well.
>
> FLAG_AVOID_UNLINK_URBS looks like it should be replaced by the newer
> ability to keep the status urb active. I believe that must have been the
> real reason for adding it, based on the commit message and the effect
> the flag will have:
>
>   commit 1487cd5e76337555737cbc55d7d83f41460d198f
>   Author: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
>   Date:   Thu Jul 30 19:41:20 2009 +0300
>
>      usbnet: allow "minidriver" to prevent urb unlinking on usbnet_stop
>
>      rndis_wlan devices freeze after running usbnet_stop several times. It appears
>      that firmware freezes in state where it does not respond to any RNDIS commands
>      and device have to be physically unplugged/replugged. This patch lets
>      minidrivers to disable unlink_urbs on usbnet_stop through new info flag.
>
>      Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
>      Cc: David Brownell <dbrownell@users.sourceforge.net>
>      Signed-off-by: John W. Linville <linville@tuxdriver.com>
>
>
>
> The rx urbs will not be resubmitted in any case, and there are of course
> no tx urbs being submitted.  So the only effect of this flag is on the
> status/interrupt urb, which I can imagine some RNDIS devices wants
> active all the time.
>
> So FLAG_AVOID_UNLINK_URBS should probably be removed and replaced calls
> to usbnet_status_start() and usbnet_status_stop().  This will require
> testing on some of the devices with the original firmware problem
> however.
>
> In any case: I do not think this flag should be considered when trying
> to make usbnet_stop behaviour saner.  It's only purpose is to
> deliberately break usbnet_stop by not actually stopping.
>
>
>> If someone could list the particular bits of dev->flags that should be
>> cleared to make sure no deferred call could reschedule usbnet_bh(),
>> etc... Well, it would be enough to clear these first and use
>> dev->flags = 0 later, after tasklet_kill() and del_timer_sync(). I
>> cannot point out these particular bits now.
>
>
> I don't think any of the flags must be cleared.  The sequence
>
>          dev_close(dev->net);
> 	usbnet_terminate_urbs(dev);
> 	usbnet_purge_paused_rxq(dev);
> 	del_timer_sync (&dev->delay);
> 	tasklet_kill (&dev->bh);
>
> should prevent any rescheduling of usbnet_bh

If so, then, I suppose, one could ignore that FLAG_AVOID_UNLINK_URBS for 
now and just move dev->flags = 0 down as you suggested and as we thought 
before.

The patch will become simpler, indeed.

>
>> Besides, it is possible, although unlikely, that new event bits will
>> be added to dev->flags in the future. And one will need to keep track
>> of these to see if they should be cleared as well. I'd prever to play
>> safer for now and clear them all.
>
> I don't think we should ever make a flag which will _have_ to be reset
> for usbnet_stop.  The only reason for clearing all flags is to reset the
> state before the next open.
>
> Yes, I see that we currently need to clear EVENT_DEV_OPEN in
> usbnet_stop, but I really don't see what this flag gives us which isn't
> already provided by netif_running().  It looks like a duplicate.
>

Regards,
Eugene

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1213021

On Mon, 2015-08-24 at 15:29 +0200, Bjørn Mork wrote:
> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:
> 
> > 19.08.2015 15:31, Bjørn Mork пишет:
> >> Eugene Shatokhin <eugene.shatokhin@rosalab.ru> writes:

> >> Stopping the tasklet rescheduling etc depends only on netif_running(),
> >> which will be false when usbnet_stop is called.  There is no need to
> >> touch dev->flags for this to happen.
> >
> > That was one of the first ideas we discussed here. Unfortunately, it
> > is probably not so simple.
> >
> > Setting dev->flags to 0 makes some delayed operations do nothing and,
> > among other things, not to reschedule usbnet_bh().
> 
> Yes, but I believe that is merely a side effect.  You should never need
> to clear multiple flags to get the desired behaviour.

Why? Is there any reason you cannot have a TX and an RX halt at the same
time?

> > As you can see in drivers/net/usb/usbnet.c, usbnet_bh() can be called
> > as a tasklet function and as a timer function in a number of
> > situations (look for the usage of dev->bh and dev->delay there).
> >
> > netif_running() is indeed false when usbnet_stop() runs, usbnet_stop()
> > also disables Tx. This seems to be enough for many cases where
> > usbnet_bh() is scheduled, but I am not so sure about the remaining
> > ones, namely:
> >
> > 1. A work function, usbnet_deferred_kevent(), may reschedule
> > usbnet_bh(). Looks like the workqueue is only stopped in
> > usbnet_disconnect(), so a work item might be processed while
> > usbnet_stop() works. Setting dev->flags to 0 makes the work function
> > do nothing, by the way. See also the comment in usbnet_stop() about
> > this.

Yes, this is the main reason the flags are collectively cleared.
We could do them all with clear_bit(). Ugly though.

> > A work item may be placed to this workqueue in a number of ways, by
> > both usbnet module and the mini-drivers. It is not too easy to track
> > all these situations.
> 
> That's an understatement :)

Yes.

> So FLAG_AVOID_UNLINK_URBS should probably be removed and replaced calls
> to usbnet_status_start() and usbnet_status_stop().  This will require
> testing on some of the devices with the original firmware problem
> however.

And there you point out the main problem.

> In any case: I do not think this flag should be considered when trying
> to make usbnet_stop behaviour saner.  It's only purpose is to
> deliberately break usbnet_stop by not actually stopping.

Yes.

	Regards
		Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web