2017-03-10 21:22:42

by Alexey Khoroshilov

Subject: z3fold: suspicious return with spinlock held

Hello!

z3fold_reclaim_page() contains the only return that may
leave the function with pool->lock spinlock held.

669 spin_lock(&pool->lock);
670 if (kref_put(&zhdr->refcount, release_z3fold_page)) {
671 atomic64_dec(&pool->pages_nr);
672 return 0;
673 }

May be we need spin_unlock(&pool->lock); just before return?


Found by Linux Driver Verification project (linuxtesting.org).

--
Thank you,
Alexey Khoroshilov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org

2017-03-10 21:34:32

by Matthew Wilcox

Subject: Re: z3fold: suspicious return with spinlock held

On Sat, Mar 11, 2017 at 12:22:12AM +0300, Alexey Khoroshilov wrote:
> Hello!
>
> z3fold_reclaim_page() contains the only return that may
> leave the function with pool->lock spinlock held.
>
> 669 spin_lock(&pool->lock);
> 670 if (kref_put(&zhdr->refcount, release_z3fold_page)) {
> 671 atomic64_dec(&pool->pages_nr);
> 672 return 0;
> 673 }
>
> May be we need spin_unlock(&pool->lock); just before return?

I would tend to agree. sparse warns about this, and also about two
other locking problems ... which I'm not sure are really problems so
much as missing annotations?

mm/z3fold.c:467:35: warning: context imbalance in 'z3fold_alloc' - unexpected unlock
mm/z3fold.c:519:26: warning: context imbalance in 'z3fold_free' - different lock contexts for basic block
mm/z3fold.c:581:12: warning: context imbalance in 'z3fold_reclaim_page' - different lock contexts for basic block

2017-03-10 21:54:51

by Alexey Khoroshilov

Subject: Re: z3fold: suspicious return with spinlock held

On 11.03.2017 00:34, Matthew Wilcox wrote:
> On Sat, Mar 11, 2017 at 12:22:12AM +0300, Alexey Khoroshilov wrote:
>> Hello!
>>
>> z3fold_reclaim_page() contains the only return that may
>> leave the function with pool->lock spinlock held.
>>
>> 669 spin_lock(&pool->lock);
>> 670 if (kref_put(&zhdr->refcount, release_z3fold_page)) {
>> 671 atomic64_dec(&pool->pages_nr);
>> 672 return 0;
>> 673 }
>>
>> May be we need spin_unlock(&pool->lock); just before return?
>
> I would tend to agree. sparse warns about this, and also about two
> other locking problems ... which I'm not sure are really problems so
> much as missing annotations?
>
> mm/z3fold.c:467:35: warning: context imbalance in 'z3fold_alloc' - unexpected unlock
> mm/z3fold.c:519:26: warning: context imbalance in 'z3fold_free' - different lock contexts for basic block
> mm/z3fold.c:581:12: warning: context imbalance in 'z3fold_reclaim_page' - different lock contexts for basic block
>

I also do not see problems in z3fold_alloc() and z3fold_free().
But I am unaware of sparse annotations that can help here.

--
Alexey

2017-03-10 22:09:06

by Vitaly Wool

Subject: Re: z3fold: suspicious return with spinlock held

Hi Alexey,

> 10 mars 2017 kl. 22:54 skrev Alexey Khoroshilov <[email protected]>:
>
>> On 11.03.2017 00:34, Matthew Wilcox wrote:
>>> On Sat, Mar 11, 2017 at 12:22:12AM +0300, Alexey Khoroshilov wrote:
>>> Hello!
>>>
>>> z3fold_reclaim_page() contains the only return that may
>>> leave the function with pool->lock spinlock held.
>>>
>>> 669 spin_lock(&pool->lock);
>>> 670 if (kref_put(&zhdr->refcount, release_z3fold_page)) {
>>> 671 atomic64_dec(&pool->pages_nr);
>>> 672 return 0;
>>> 673 }
>>>
>>> May be we need spin_unlock(&pool->lock); just before return?
Looks so, thanks for the pointer. I'm currently commuting but will check it thoroughly tomorrow for sure.

~vitaly

>>
>> I would tend to agree. sparse warns about this, and also about two
>> other locking problems ... which I'm not sure are really problems so
>> much as missing annotations?
>>
>> mm/z3fold.c:467:35: warning: context imbalance in 'z3fold_alloc' - unexpected unlock
>> mm/z3fold.c:519:26: warning: context imbalance in 'z3fold_free' - different lock contexts for basic block
>> mm/z3fold.c:581:12: warning: context imbalance in 'z3fold_reclaim_page' - different lock contexts for basic block
>>
>
> I also do not see problems in z3fold_alloc() and z3fold_free().
> But I am unaware of sparse annotations that can help here.
>
> --
> Alexey