2. The second race is on dev->flags.
dev->flags is set to 0 here:
*0 usbnet_stop (usbnet.c:816)
/* deferred work (task, timer, softirq) must also stop.
* can't flush_scheduled_work() until we drop rtnl (later),
* else workers could deadlock; so make workers a NOP.
*/
dev->flags = 0;
del_timer_sync (&dev->delay);
tasklet_kill (&dev->bh);
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);
It seems, setting dev->flags to 0 is not necessarily atomic w.r.t.
clear_bit() and other bit operations with dev->flags. It is safer to
make it atomic and this way, make the race harmless.
While at it, the checking of EVENT_NO_RUNTIME_PM bit of dev->flags in
usbnet_stop() was fixed too: the bit should be checked before dev->flags
is cleared.
The fix for this is excessive.
Instead of all of this madness, looping over expensive clear_bit()
atomics, just do whatever it takes to make sure that usbnet_bh() is
quiesced and cannot execute any more. Then you can safely clear
dev->flags normally.
If I understand it correctly, it is to make sure usbnet_bh() is not
scheduled again that dev->flags should be set to 0 first, one way or
another. That is what this madness is for.
Assuming there is a race which may reorder these, exactly what
difference does it make wrt EVENT_RX_KILL if you do
a) clear_bit(EVENT_RX_KILL, &dev->flags);
dev->flags = 0;
or
b) dev->flags = 0;
clear_bit(EVENT_RX_KILL, &dev->flags);
AFAICS, the result will be a cleared EVENT_RX_KILL bit in either case.
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)
Bjørn
2. The second race is on dev->flags.
dev->flags is set to 0 here:
*0 usbnet_stop (usbnet.c:816)
/* deferred work (task, timer, softirq) must also stop.
* can't flush_scheduled_work() until we drop rtnl (later),
* else workers could deadlock; so make workers a NOP.
*/
dev->flags = 0;
del_timer_sync (&dev->delay);
tasklet_kill (&dev->bh);
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);
It seems, setting dev->flags to 0 is not necessarily atomic w.r.t.
clear_bit() and other bit operations with dev->flags. It is safer to
make it atomic and this way, make the race harmless.
While at it, the checking of EVENT_NO_RUNTIME_PM bit of dev->flags in
usbnet_stop() was fixed too: the bit should be checked before dev->flags
is cleared.
The fix for this is excessive.
Instead of all of this madness, looping over expensive clear_bit()
atomics, just do whatever it takes to make sure that usbnet_bh() is
quiesced and cannot execute any more. Then you can safely clear
dev->flags normally.
If I understand it correctly, it is to make sure usbnet_bh() is not
scheduled again that dev->flags should be set to 0 first, one way or
another. That is what this madness is for.
Assuming there is a race which may reorder these, exactly what
difference does it make wrt EVENT_RX_KILL if you do
a) clear_bit(EVENT_RX_KILL, &dev->flags);
dev->flags = 0;
or
b) dev->flags = 0;
clear_bit(EVENT_RX_KILL, &dev->flags);
AFAICS, the result will be a cleared EVENT_RX_KILL bit in either case.
Thanks for the review!
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.
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.
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.
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.
Is this really possible?
Stores really are "atomic" in the sense that the do their update
in one indivisible operation.
Atomic operations like clear_bit also will behave that way.
If a clear_bit is in progress, the "dev->flags=0" store will not be
able to grab the cache line exclusively until the clear_bit is done.
So I think the above sequent of events is completely impossible. Once
a clear_bit starts, a write by another foreign agent on the bus is
absolutely impossible to legally occur until the clear_bit completes.
I think this is a non-issue.
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.
Is this really possible?
Stores really are "atomic" in the sense that the do their update
in one indivisible operation.
Provided you use ACCESS_ONCE or WRITE_ONCE or whatever people like to
call it now.
Atomic operations like clear_bit also will behave that way.
Are you certain about that? I couldn't find any mention of it in
Documentation/atomic_ops.txt.
In theory, an architecture could implement atomic bit operations using
a spinlock to insure atomicity. I don't know if any architectures do
this, but if they do then the scenario above could arise.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
Is this really possible?
On x86, it is not possible, so this is not a problem. Perhaps, for ARM
too. As for the other architectures supported by the kernel - not sure,
no common guarantees, it seems. Anyway, this is not a critical issue, I
agree.
OK, let us leave things as they are for this one and fix the rest.
Stores really are "atomic" in the sense that the do their update
in one indivisible operation.
Atomic operations like clear_bit also will behave that way.
If a clear_bit is in progress, the "dev->flags=0" store will not be
able to grab the cache line exclusively until the clear_bit is done.
So I think the above sequent of events is completely impossible. Once
a clear_bit starts, a write by another foreign agent on the bus is
absolutely impossible to legally occur until the clear_bit completes.
I think this is a non-issue.
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.
Is this really possible?
Stores really are "atomic" in the sense that the do their update
in one indivisible operation.
Provided you use ACCESS_ONCE or WRITE_ONCE or whatever people like to
call it now.
quoted
Atomic operations like clear_bit also will behave that way.
Are you certain about that? I couldn't find any mention of it in
Documentation/atomic_ops.txt.
In theory, an architecture could implement atomic bit operations using
a spinlock to insure atomicity. I don't know if any architectures do
this, but if they do then the scenario above could arise.
Now that I see this in writing, I realize it's not possible after all.
clear_bit() et al. will work with a single unsigned long, which doesn't
leave any place for spinlocks or other mechanisms. I was thinking of
atomic_t.
So never mind...
Alan Stern
From: David Miller <davem@davemloft.net> Date: 2015-08-24 18:35:43
From: Alan Stern <stern@rowland.harvard.edu>
Date: Mon, 24 Aug 2015 14:06:15 -0400 (EDT)
On Mon, 24 Aug 2015, David Miller wrote:
quoted
Atomic operations like clear_bit also will behave that way.
Are you certain about that? I couldn't find any mention of it in
Documentation/atomic_ops.txt.
In theory, an architecture could implement atomic bit operations using
a spinlock to insure atomicity. I don't know if any architectures do
this, but if they do then the scenario above could arise.
Indeed, we do have platforms like 32-bit sparc and parisc that do this.
So, taking that into consideration, this is a bit unfortunate and on
such platforms we do have this problem.
From: Oliver Neukum <oneukum@suse.com> Date: 2015-08-25 12:37:59
On Mon, 2015-08-24 at 14:21 -0400, Alan Stern wrote:
quoted
In theory, an architecture could implement atomic bit operations
using
quoted
a spinlock to insure atomicity. I don't know if any architectures
do
quoted
this, but if they do then the scenario above could arise.
Now that I see this in writing, I realize it's not possible after
all.
clear_bit() et al. will work with a single unsigned long, which
doesn't
leave any place for spinlocks or other mechanisms. I was thinking of
atomic_t.
Refuting yourself you are making the assumption that the lock has
to be inside the data structure. That is not true.
Regards
Oliver