[PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN

[PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN

[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

Re: [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN

[Christoph Hellwig]

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.

Re: [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN

[Waiman Long]

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

RE: [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN

[David Laight]

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)

Re: [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN

[Waiman Long]

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

Re: [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN

[Peter Zijlstra]

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!

Re: [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN

[Waiman Long]

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

RE: [PATCH] locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN

[David Laight]

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)