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

[Luc Van Oostenryck <luc.vanoostenryck@gmail.com>]

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