lock checking issues (was: Re: [PATCH v3] cifs: Fix leak when handling lease break for cached root fid)

lock checking issues (was: Re: [PATCH v3] cifs: Fix leak when handling lease break for cached root fid)

[Aurélien Aptel]

CC-ing linux-sparse

We are seeing a lock context imbalance warning which we can't get rid
of after applying a patch moving locking around across function boundaries.

For context see:
https://lore.kernel.org/linux-cifs/20200702164411.108672-1-paul@darkrain42.org/T/#u

Paul Aurich <paul@darkrain42.org> writes:
> On 2020-07-06 10:30:27 +0200, Aurélien Aptel wrote:
>>Paul Aurich <paul@darkrain42.org> writes:
>>> Changes since v2:
>>>    - address sparse lock warnings by inlining smb2_tcon_has_lease and
>>>      hardcoding some return values (seems to help sparse's analysis)
>>
>>Ah, I think the issue is not the inlining but rather you need to
>>instruct sparse that smb2_tcon_hash_lease is expected to release the
>>lock.
>>
>>https://www.kernel.org/doc/html/v4.12/dev-tools/sparse.html#using-sparse-for-lock-checking
>>
>>Probably with __releases somewhere in the func prototype.
>
> I tried various iterations of that without finding one that seems to work. 
> I suspect it's because the unlocking is _conditional_.

Hm could be it...

> w/o the inline and with __releases(&cifs_tcp_ses_lock):
>
> fs/cifs/smb2misc.c:508:1: warning: context imbalance in 'smb2_tcon_has_lease' 
> - different lock contexts for basic block
> fs/cifs/smb2misc.c:612:25: warning: context imbalance in 
> 'smb2_is_valid_lease_break'- different lock contexts for basic block
>
>
> Digging further, I found __acquire and __release (not plural), which can be 
> used in individual blocks. The following seems to silence the sparse warnings 
> - does this seem valid?

To be honnest I'm not sure, these seem counterproductive. If you are
indicating you are acquiring X but lock Y the next line it feels like we
are fighting the tool instead of letting it help us.

> @@ -504,7 +504,7 @@ cifs_ses_oplock_break(struct work_struct *work)
>   	kfree(lw);
>   }
>   
> -static inline bool
> +static bool
>   smb2_tcon_has_lease(struct cifs_tcon *tcon, struct smb2_lease_break *rsp)
>   {
>   	bool found;
> @@ -521,6 +521,7 @@ smb2_tcon_has_lease(struct cifs_tcon *tcon, struct smb2_lease_break *rsp)
>   
>   	lease_state = le32_to_cpu(rsp->NewLeaseState);
>   
> +	__acquire(cifs_tcp_ses_lock);

Should it have a "&" here?

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Re: lock checking issues (was: Re: [PATCH v3] cifs: Fix leak when handling lease break for cached root fid)

[Luc Van Oostenryck]

On Tue, Jul 07, 2020 at 09:03:17AM +0200, Aurélien Aptel wrote:
> 
> CC-ing linux-sparse
> 
> We are seeing a lock context imbalance warning which we can't get rid
> of after applying a patch moving locking around across function boundaries.
> 
> For context see:
> https://lore.kernel.org/linux-cifs/20200702164411.108672-1-paul@darkrain42.org/T/#u
> 
> Paul Aurich <paul@darkrain42.org> writes:
> > On 2020-07-06 10:30:27 +0200, Aurélien Aptel wrote:
> >>Paul Aurich <paul@darkrain42.org> writes:
> >>> Changes since v2:
> >>>    - address sparse lock warnings by inlining smb2_tcon_has_lease and
> >>>      hardcoding some return values (seems to help sparse's analysis)
> >>
> >>Ah, I think the issue is not the inlining but rather you need to
> >>instruct sparse that smb2_tcon_hash_lease is expected to release the
> >>lock.
> >>
> >>https://www.kernel.org/doc/html/v4.12/dev-tools/sparse.html#using-sparse-for-lock-checking
> >>
> >>Probably with __releases somewhere in the func prototype.

Could be both:
 * if it's inlined, then its users can deduce its locking signature.
 * if it's not inlined, then the locking signature must be given in
   in the prototype (with __acquires(), __releases() or __must_hold()).

> > I tried various iterations of that without finding one that seems to work. 
> > I suspect it's because the unlocking is _conditional_.
> 
> Hm could be it...

It's possible indeed. In general, conditional locking can't be statically
checked. The problem is undecidable and sparse doesn't try to check.
What sparse is checking is that all paths are 'balanced.

So, with code like:
	if (cond)
		lock();
	do_stuff;
	if (cond)
		unlock();

sparse will probably have problems. It's possible that everything id fine
if the condition is known and the remaining of the code is simple enough
but most often you will have a warning about context imbalance.

The way to avoid any problem is to write instead:

	if (cond) {
		lock();
		do_stuff;
		unlock();
	} else {
		do_stuff;
	}

which is not too bad when do_stuff can be put in a separate function,
inlined or not.

> > w/o the inline and with __releases(&cifs_tcp_ses_lock):
> >
> > fs/cifs/smb2misc.c:508:1: warning: context imbalance in 'smb2_tcon_has_lease' 
> > - different lock contexts for basic block
> > fs/cifs/smb2misc.c:612:25: warning: context imbalance in 
> > 'smb2_is_valid_lease_break'- different lock contexts for basic block
> >
> >
> > Digging further, I found __acquire and __release (not plural), which can be 
> > used in individual blocks. The following seems to silence the sparse warnings 
> > - does this seem valid?
> 
> To be honnest I'm not sure, these seem counterproductive. If you are
> indicating you are acquiring X but lock Y the next line it feels like we
> are fighting the tool instead of letting it help us.

__acquire() & __release() should only be used by locking primitives.

> > +	__acquire(cifs_tcp_ses_lock);
> 
> Should it have a "&" here?

In truth, it won't matter because the 'context' argument is only used
as a kind of documentation. The general use is to use it like the
argument to one of the lock function: as a pointer.


If needed, I can look more concretely at the problems here later
this evening but it will help a lot if would have:
* a tree with the code in question
* the config that is used (but I suspect it doesn't matter much here).

Cheers,
-- Luc

Re: lock checking issues (was: Re: [PATCH v3] cifs: Fix leak when handling lease break for cached root fid)

[Paul Aurich]

On 2020-07-07 15:05:09 +0200, Luc Van Oostenryck wrote:
>> To be honnest I'm not sure, these seem counterproductive. If you are
>> indicating you are acquiring X but lock Y the next line it feels like we
>> are fighting the tool instead of letting it help us.
>
>__acquire() & __release() should only be used by locking primitives.

Alright, thanks! (I did see several other locations in the kernel that seemed 
to be doing this, though I agree with Aurélien that it didn't feel right to be 
fighting sparse, and it's possible my attempt was more egregious.)

I will try to find a better way to organize the functionality that satisfies 
both the functionality and sparse.

Thanks,
~Paul