| Message ID | 20200702164411.108672-1-paul@darkrain42.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] cifs: Fix leak when handling lease break for cached root fid | expand |
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
diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c index 6a39451973f8..8919df9d7dfd 100644 --- a/fs/cifs/smb2misc.c +++ b/fs/cifs/smb2misc.c @@ -504,9 +504,8 @@ cifs_ses_oplock_break(struct work_struct *work) kfree(lw); } -static bool -smb2_tcon_has_lease(struct cifs_tcon *tcon, struct smb2_lease_break *rsp, - struct smb2_lease_break_work *lw) +static inline bool +smb2_tcon_has_lease(struct cifs_tcon *tcon, struct smb2_lease_break *rsp) { bool found; __u8 lease_state; @@ -516,9 +515,13 @@ smb2_tcon_has_lease(struct cifs_tcon *tcon, struct smb2_lease_break *rsp, struct cifsInodeInfo *cinode; int ack_req = le32_to_cpu(rsp->Flags & SMB2_NOTIFY_BREAK_LEASE_FLAG_ACK_REQUIRED); + struct smb2_lease_break_work *lw; + struct tcon_link *tlink; + __u8 lease_key[SMB2_LEASE_KEY_SIZE]; lease_state = le32_to_cpu(rsp->NewLeaseState); + spin_lock(&tcon->open_file_lock); list_for_each(tmp, &tcon->openFileList) { cfile = list_entry(tmp, struct cifsFileInfo, tlist); cinode = CIFS_I(d_inode(cfile->dentry)); @@ -542,7 +545,8 @@ smb2_tcon_has_lease(struct cifs_tcon *tcon, struct smb2_lease_break *rsp, cfile->oplock_level = lease_state; cifs_queue_oplock_break(cfile); - kfree(lw); + spin_unlock(&tcon->open_file_lock); + spin_unlock(&cifs_tcp_ses_lock); return true; } @@ -554,10 +558,9 @@ smb2_tcon_has_lease(struct cifs_tcon *tcon, struct smb2_lease_break *rsp, if (!found && ack_req) { found = true; - memcpy(lw->lease_key, open->lease_key, + memcpy(lease_key, open->lease_key, SMB2_LEASE_KEY_SIZE); - lw->tlink = cifs_get_tlink(open->tlink); - queue_work(cifsiod_wq, &lw->lease_break); + tlink = cifs_get_tlink(open->tlink); } cifs_dbg(FYI, "found in the pending open list\n"); @@ -567,25 +570,33 @@ smb2_tcon_has_lease(struct cifs_tcon *tcon, struct smb2_lease_break *rsp, open->oplock = lease_state; } - return found; -} - -static bool -smb2_is_valid_lease_break(char *buffer) -{ - struct smb2_lease_break *rsp = (struct smb2_lease_break *)buffer; - struct list_head *tmp, *tmp1, *tmp2; - struct TCP_Server_Info *server; - struct cifs_ses *ses; - struct cifs_tcon *tcon; - struct smb2_lease_break_work *lw; + spin_unlock(&tcon->open_file_lock); + if (!found) + return false; + spin_unlock(&cifs_tcp_ses_lock); lw = kmalloc(sizeof(struct smb2_lease_break_work), GFP_KERNEL); - if (!lw) - return false; + if (!lw) { + cifs_put_tlink(tlink); + return true; + } INIT_WORK(&lw->lease_break, cifs_ses_oplock_break); + lw->tlink = tlink; lw->lease_state = rsp->NewLeaseState; + memcpy(lw->lease_key, lease_key, SMB2_LEASE_KEY_SIZE); + queue_work(cifsiod_wq, &lw->lease_break); + return true; +} + +static bool +smb2_is_valid_lease_break(char *buffer) +{ + struct smb2_lease_break *rsp = (struct smb2_lease_break *)buffer; + struct list_head *tmp, *tmp1, *tmp2; + struct TCP_Server_Info *server; + struct cifs_ses *ses; + struct cifs_tcon *tcon; cifs_dbg(FYI, "Checking for lease break\n"); @@ -600,15 +611,11 @@ smb2_is_valid_lease_break(char *buffer) list_for_each(tmp2, &ses->tcon_list) { tcon = list_entry(tmp2, struct cifs_tcon, tcon_list); - spin_lock(&tcon->open_file_lock); cifs_stats_inc( &tcon->stats.cifs_stats.num_oplock_brks); - if (smb2_tcon_has_lease(tcon, rsp, lw)) { - spin_unlock(&tcon->open_file_lock); - spin_unlock(&cifs_tcp_ses_lock); + if (smb2_tcon_has_lease(tcon, rsp)) { return true; } - spin_unlock(&tcon->open_file_lock); if (tcon->crfid.is_valid && !memcmp(rsp->LeaseKey, @@ -625,7 +632,6 @@ smb2_is_valid_lease_break(char *buffer) } } spin_unlock(&cifs_tcp_ses_lock); - kfree(lw); cifs_dbg(FYI, "Can not process lease break - no lease matched\n"); return false; }
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(-)