* [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN
@ 2020-01-14 19:03 Waiman Long
2020-01-15 6:50 ` Christoph Hellwig
0 siblings, 1 reply; 8+ messages in thread
From: Waiman Long @ 2020-01-14 19:03 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon
Cc: linux-kernel, Christoph Hellwig, stable, Waiman Long
The commit 91d2a812dfb9 ("locking/rwsem: Make handoff writer
optimistically spin on owner") will allow a recently woken up waiting
writer to spin on the owner. Unfortunately, if the owner happens to be
RWSEM_OWNER_UNKNOWN, the code will incorrectly spin on it leading to a
kernel crash. This is fixed by passing the proper non-spinnable bits
to rwsem_spin_on_owner() so that RWSEM_OWNER_UNKNOWN will be treated
as a non-spinnable target.
Fixes: 91d2a812dfb9 ("locking/rwsem: Make handoff writer optimistically spin on owner")
Reported-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Waiman Long <longman@redhat.com>
---
kernel/locking/rwsem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index 44e68761f432..1dd3d53f43c3 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -1227,7 +1227,7 @@ rwsem_down_write_slowpath(struct rw_semaphore *sem, int state)
* without sleeping.
*/
if ((wstate == WRITER_HANDOFF) &&
- (rwsem_spin_on_owner(sem, 0) == OWNER_NULL))
+ rwsem_spin_on_owner(sem, RWSEM_NONSPINNABLE) == OWNER_NULL)
goto trylock_again;
/* Block until there are no active lockers. */
--
2.18.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN
2020-01-14 19:03 [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN Waiman Long
@ 2020-01-15 6:50 ` Christoph Hellwig
2020-01-15 14:27 ` Waiman Long
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Christoph Hellwig @ 2020-01-15 6:50 UTC (permalink / raw)
To: Waiman Long
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, linux-kernel,
Christoph Hellwig, stable
On Tue, Jan 14, 2020 at 02:03:03PM -0500, Waiman Long wrote:
> The commit 91d2a812dfb9 ("locking/rwsem: Make handoff writer
> optimistically spin on owner") will allow a recently woken up waiting
> writer to spin on the owner. Unfortunately, if the owner happens to be
> RWSEM_OWNER_UNKNOWN, the code will incorrectly spin on it leading to a
> kernel crash. This is fixed by passing the proper non-spinnable bits
> to rwsem_spin_on_owner() so that RWSEM_OWNER_UNKNOWN will be treated
> as a non-spinnable target.
>
> Fixes: 91d2a812dfb9 ("locking/rwsem: Make handoff writer optimistically spin on owner")
>
> Reported-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Waiman Long <longman@redhat.com>
This survives all the tests that showed the problems with the original
code:
Tested-by: Christoph Hellwig <hch@lst.de>
> if ((wstate == WRITER_HANDOFF) &&
> - (rwsem_spin_on_owner(sem, 0) == OWNER_NULL))
> + rwsem_spin_on_owner(sem, RWSEM_NONSPINNABLE) == OWNER_NULL)
Nit: the inner braces in the first half of the conditional aren't required
either.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN
2020-01-15 6:50 ` Christoph Hellwig
@ 2020-01-15 14:27 ` Waiman Long
2020-01-15 15:16 ` David Laight
2020-01-15 15:28 ` Waiman Long
2020-01-15 15:37 ` Peter Zijlstra
2 siblings, 1 reply; 8+ messages in thread
From: Waiman Long @ 2020-01-15 14:27 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, linux-kernel, stable
On 1/15/20 1:50 AM, Christoph Hellwig wrote:
> On Tue, Jan 14, 2020 at 02:03:03PM -0500, Waiman Long wrote:
>> The commit 91d2a812dfb9 ("locking/rwsem: Make handoff writer
>> optimistically spin on owner") will allow a recently woken up waiting
>> writer to spin on the owner. Unfortunately, if the owner happens to be
>> RWSEM_OWNER_UNKNOWN, the code will incorrectly spin on it leading to a
>> kernel crash. This is fixed by passing the proper non-spinnable bits
>> to rwsem_spin_on_owner() so that RWSEM_OWNER_UNKNOWN will be treated
>> as a non-spinnable target.
>>
>> Fixes: 91d2a812dfb9 ("locking/rwsem: Make handoff writer optimistically spin on owner")
>>
>> Reported-by: Christoph Hellwig <hch@lst.de>
>> Signed-off-by: Waiman Long <longman@redhat.com>
> This survives all the tests that showed the problems with the original
> code:
>
> Tested-by: Christoph Hellwig <hch@lst.de>
>
>> if ((wstate == WRITER_HANDOFF) &&
>> - (rwsem_spin_on_owner(sem, 0) == OWNER_NULL))
>> + rwsem_spin_on_owner(sem, RWSEM_NONSPINNABLE) == OWNER_NULL)
> Nit: the inner braces in the first half of the conditional aren't required
> either.
I typically over-parenthesize the code to make it easier to read as we
don't need to think too much about operator precedence to see if it is
doing the right thing. I remove the 2nd parentheses to avoid breaking
the 80-colnum limit.
Cheers,
Longman
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN
2020-01-15 14:27 ` Waiman Long
@ 2020-01-15 15:16 ` David Laight
2020-01-15 15:47 ` Waiman Long
0 siblings, 1 reply; 8+ messages in thread
From: David Laight @ 2020-01-15 15:16 UTC (permalink / raw)
To: 'Waiman Long', Christoph Hellwig
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, linux-kernel, stable
From: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> On Behalf Of Waiman Long
> Sent: 15 January 2020 14:27
...
> >> if ((wstate == WRITER_HANDOFF) &&
> >> - (rwsem_spin_on_owner(sem, 0) == OWNER_NULL))
> >> + rwsem_spin_on_owner(sem, RWSEM_NONSPINNABLE) == OWNER_NULL)
> > Nit: the inner braces in the first half of the conditional aren't required
> > either.
>
> I typically over-parenthesize the code to make it easier to read as we
> don't need to think too much about operator precedence to see if it is
> doing the right thing.
The problem is it actually makes it harder to read.
It is difficult for the 'mark 1 eyeball' to follow lots of sets of brackets.
Since == (etc) are the lowest priority operators (apart from ?:) they
never need ().
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN
2020-01-15 6:50 ` Christoph Hellwig
2020-01-15 14:27 ` Waiman Long
@ 2020-01-15 15:28 ` Waiman Long
2020-01-15 15:37 ` Peter Zijlstra
2 siblings, 0 replies; 8+ messages in thread
From: Waiman Long @ 2020-01-15 15:28 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, linux-kernel, stable
On 1/15/20 1:50 AM, Christoph Hellwig wrote:
> On Tue, Jan 14, 2020 at 02:03:03PM -0500, Waiman Long wrote:
>> The commit 91d2a812dfb9 ("locking/rwsem: Make handoff writer
>> optimistically spin on owner") will allow a recently woken up waiting
>> writer to spin on the owner. Unfortunately, if the owner happens to be
>> RWSEM_OWNER_UNKNOWN, the code will incorrectly spin on it leading to a
>> kernel crash. This is fixed by passing the proper non-spinnable bits
>> to rwsem_spin_on_owner() so that RWSEM_OWNER_UNKNOWN will be treated
>> as a non-spinnable target.
>>
>> Fixes: 91d2a812dfb9 ("locking/rwsem: Make handoff writer optimistically spin on owner")
>>
>> Reported-by: Christoph Hellwig <hch@lst.de>
>> Signed-off-by: Waiman Long <longman@redhat.com>
> This survives all the tests that showed the problems with the original
> code:
>
> Tested-by: Christoph Hellwig <hch@lst.de>
>
Thanks for the testing.
>> if ((wstate == WRITER_HANDOFF) &&
>> - (rwsem_spin_on_owner(sem, 0) == OWNER_NULL))
>> + rwsem_spin_on_owner(sem, RWSEM_NONSPINNABLE) == OWNER_NULL)
> Nit: the inner braces in the first half of the conditional aren't required
> either.
Yes, it is inconsistent and so is not good. I will post a v2 patch to
fix that.
Cheers,
Longman
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN
2020-01-15 6:50 ` Christoph Hellwig
2020-01-15 14:27 ` Waiman Long
2020-01-15 15:28 ` Waiman Long
@ 2020-01-15 15:37 ` Peter Zijlstra
2 siblings, 0 replies; 8+ messages in thread
From: Peter Zijlstra @ 2020-01-15 15:37 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Waiman Long, Ingo Molnar, Will Deacon, linux-kernel, stable
On Wed, Jan 15, 2020 at 07:50:55AM +0100, Christoph Hellwig wrote:
> On Tue, Jan 14, 2020 at 02:03:03PM -0500, Waiman Long wrote:
> > The commit 91d2a812dfb9 ("locking/rwsem: Make handoff writer
> > optimistically spin on owner") will allow a recently woken up waiting
> > writer to spin on the owner. Unfortunately, if the owner happens to be
> > RWSEM_OWNER_UNKNOWN, the code will incorrectly spin on it leading to a
> > kernel crash. This is fixed by passing the proper non-spinnable bits
> > to rwsem_spin_on_owner() so that RWSEM_OWNER_UNKNOWN will be treated
> > as a non-spinnable target.
> >
> > Fixes: 91d2a812dfb9 ("locking/rwsem: Make handoff writer optimistically spin on owner")
> >
> > Reported-by: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Waiman Long <longman@redhat.com>
>
> This survives all the tests that showed the problems with the original
> code:
>
> Tested-by: Christoph Hellwig <hch@lst.de>
Thanks!
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN
2020-01-15 15:16 ` David Laight
@ 2020-01-15 15:47 ` Waiman Long
2020-01-15 16:01 ` David Laight
0 siblings, 1 reply; 8+ messages in thread
From: Waiman Long @ 2020-01-15 15:47 UTC (permalink / raw)
To: David Laight, Christoph Hellwig
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, linux-kernel, stable
On 1/15/20 10:16 AM, David Laight wrote:
> From: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> On Behalf Of Waiman Long
>> Sent: 15 January 2020 14:27
> ...
>>>> if ((wstate == WRITER_HANDOFF) &&
>>>> - (rwsem_spin_on_owner(sem, 0) == OWNER_NULL))
>>>> + rwsem_spin_on_owner(sem, RWSEM_NONSPINNABLE) == OWNER_NULL)
>>> Nit: the inner braces in the first half of the conditional aren't required
>>> either.
>> I typically over-parenthesize the code to make it easier to read as we
>> don't need to think too much about operator precedence to see if it is
>> doing the right thing.
> The problem is it actually makes it harder to read.
> It is difficult for the 'mark 1 eyeball' to follow lots of sets of brackets.
> Since == (etc) are the lowest priority operators (apart from ?:) they
> never need ().
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
It depends. I find it hard to read an expression with "&" and "&&"
without parentheses. Anyway, I will admit that the above code is
inconsistent in term of how parentheses are used. So I will change that.
Cheers,
Longman
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN
2020-01-15 15:47 ` Waiman Long
@ 2020-01-15 16:01 ` David Laight
0 siblings, 0 replies; 8+ messages in thread
From: David Laight @ 2020-01-15 16:01 UTC (permalink / raw)
To: 'Waiman Long', Christoph Hellwig
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, linux-kernel, stable
From: Waiman Long
> Sent: 15 January 2020 15:48
...
> It depends. I find it hard to read an expression with "&" and "&&"
> without parentheses. Anyway, I will admit that the above code is
> inconsistent in term of how parentheses are used. So I will change that.
Conditionals containing fragments like (a == b && c == d && ...)
are much easier to read without any extra ().
The only problem with && is that when K&R added it to C they didn't
change the priority of & to be higher than == (where it should be).
At that time they could have changed all the existing code...
Modern compilers do warn about (a == b & c).
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-01-15 16:01 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-14 19:03 [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN Waiman Long
2020-01-15 6:50 ` Christoph Hellwig
2020-01-15 14:27 ` Waiman Long
2020-01-15 15:16 ` David Laight
2020-01-15 15:47 ` Waiman Long
2020-01-15 16:01 ` David Laight
2020-01-15 15:28 ` Waiman Long
2020-01-15 15:37 ` Peter Zijlstra
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.