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

["Aurélien Aptel" <aaptel@suse.com>]

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)