[v3] cifs: Fix leak when handling lease break for cached root fid

Handling a lease break for the cached root didn't free the
smb2_lease_break_work allocation, resulting in a leak:

    unreferenced object 0xffff98383a5af480 (size 128):
      comm "cifsd", pid 684, jiffies 4294936606 (age 534.868s)
      hex dump (first 32 bytes):
        c0 ff ff ff 1f 00 00 00 88 f4 5a 3a 38 98 ff ff  ..........Z:8...
        88 f4 5a 3a 38 98 ff ff 80 88 d6 8a ff ff ff ff  ..Z:8...........
      backtrace:
        [<0000000068957336>] smb2_is_valid_oplock_break+0x1fa/0x8c0
        [<0000000073b70b9e>] cifs_demultiplex_thread+0x73d/0xcc0
        [<00000000905fa372>] kthread+0x11c/0x150
        [<0000000079378e4e>] ret_from_fork+0x22/0x30

Avoid this leak by only allocating when necessary.

Fixes: a93864d93977 ("cifs: add lease tracking to the cached root fid")
Signed-off-by: Paul Aurich <paul@darkrain42.org>
CC: Stable <stable@vger.kernel.org> # v4.18+
---
Changes since v2:
   - address sparse lock warnings by inlining smb2_tcon_has_lease and
     hardcoding some return values (seems to help sparse's analysis)

 fs/cifs/smb2misc.c | 60 +++++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

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.

Cheers,

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_.

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?

@@ -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);
  	spin_lock(&tcon->open_file_lock);
  	list_for_each(tmp, &tcon->openFileList) {
  		cfile = list_entry(tmp, struct cifsFileInfo, tlist);
@@ -571,8 +572,7 @@ smb2_tcon_has_lease(struct cifs_tcon *tcon, struct smb2_lease_break *rsp)
  	}
  
  	spin_unlock(&tcon->open_file_lock);
-	if (!found)
-		return false;
+	if (found) {
  		spin_unlock(&cifs_tcp_ses_lock);
  
  		lw = kmalloc(sizeof(struct smb2_lease_break_work), GFP_KERNEL);
@@ -586,7 +586,12 @@ smb2_tcon_has_lease(struct cifs_tcon *tcon, struct smb2_lease_break *rsp)
  		lw->lease_state = rsp->NewLeaseState;
  		memcpy(lw->lease_key, lease_key, SMB2_LEASE_KEY_SIZE);
  		queue_work(cifsiod_wq, &lw->lease_break);
-	return true;
+	} else {
+		/* for sparse */
+		__release(&cifs_tcp_ses_lock);
+	}
+
+	return found;
  }
  
  static bool
@@ -614,6 +619,8 @@ smb2_is_valid_lease_break(char *buffer)
  				cifs_stats_inc(
  				    &tcon->stats.cifs_stats.num_oplock_brks);
  				if (smb2_tcon_has_lease(tcon, rsp)) {
+					/* Unlocked in smb2_tcon_has_lease */
+					__release(&cifs_tcp_ses_lock);
  					return true;
  				}


~Paul

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,

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

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