| Message ID | 20200630023003.1858066-1-paul@darkrain42.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] cifs: Fix leak when handling lease break for cached root fid | expand |
Great, thanks.
Reviewed-by: Aurelien Aptel <aaptel@suse.com>
Cheers,
I get the following sparse warning compiling this patch (make C=1 -C /usr/src/linux-headers-`uname -r` M=`pwd` modules CF=-D__CHECK_ENDIAN__) CHECK /home/smfrench/cifs-2.6/fs/cifs/smb2misc.c /home/smfrench/cifs-2.6/fs/cifs/smb2misc.c:542:24: warning: context imbalance in 'smb2_tcon_has_lease' - unexpected unlock /home/smfrench/cifs-2.6/fs/cifs/smb2misc.c:594:1: warning: context imbalance in 'smb2_is_valid_lease_break' - wrong count at exit CC [M] /home/smfrench/cifs-2.6/fs/cifs/smb2misc.o On Mon, Jun 29, 2020 at 9:30 PM Paul Aurich <paul@darkrain42.org> wrote: > > 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+ > --- > fs/cifs/smb2misc.c | 47 ++++++++++++++++++++++++++-------------------- > 1 file changed, 27 insertions(+), 20 deletions(-) > > diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c > index 6a39451973f8..570c0521fc3c 100644 > --- a/fs/cifs/smb2misc.c > +++ b/fs/cifs/smb2misc.c > @@ -505,8 +505,7 @@ cifs_ses_oplock_break(struct work_struct *work) > } > > static bool > -smb2_tcon_has_lease(struct cifs_tcon *tcon, struct smb2_lease_break *rsp, > - struct smb2_lease_break_work *lw) > +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,6 +570,23 @@ smb2_tcon_has_lease(struct cifs_tcon *tcon, struct smb2_lease_break *rsp, > open->oplock = lease_state; > } > > + spin_unlock(&tcon->open_file_lock); > + if (found) { > + spin_unlock(&cifs_tcp_ses_lock); > + > + lw = kmalloc(sizeof(struct smb2_lease_break_work), GFP_KERNEL); > + 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 found; > } > > @@ -578,14 +598,6 @@ smb2_is_valid_lease_break(char *buffer) > struct TCP_Server_Info *server; > struct cifs_ses *ses; > struct cifs_tcon *tcon; > - struct smb2_lease_break_work *lw; > - > - lw = kmalloc(sizeof(struct smb2_lease_break_work), GFP_KERNEL); > - if (!lw) > - return false; > - > - INIT_WORK(&lw->lease_break, cifs_ses_oplock_break); > - lw->lease_state = rsp->NewLeaseState; > > cifs_dbg(FYI, "Checking for lease break\n"); > > @@ -600,15 +612,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 +633,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; > } > -- > 2.27.0 >
diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c index 6a39451973f8..570c0521fc3c 100644 --- a/fs/cifs/smb2misc.c +++ b/fs/cifs/smb2misc.c @@ -505,8 +505,7 @@ cifs_ses_oplock_break(struct work_struct *work) } static bool -smb2_tcon_has_lease(struct cifs_tcon *tcon, struct smb2_lease_break *rsp, - struct smb2_lease_break_work *lw) +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,6 +570,23 @@ smb2_tcon_has_lease(struct cifs_tcon *tcon, struct smb2_lease_break *rsp, open->oplock = lease_state; } + spin_unlock(&tcon->open_file_lock); + if (found) { + spin_unlock(&cifs_tcp_ses_lock); + + lw = kmalloc(sizeof(struct smb2_lease_break_work), GFP_KERNEL); + 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 found; } @@ -578,14 +598,6 @@ smb2_is_valid_lease_break(char *buffer) struct TCP_Server_Info *server; struct cifs_ses *ses; struct cifs_tcon *tcon; - struct smb2_lease_break_work *lw; - - lw = kmalloc(sizeof(struct smb2_lease_break_work), GFP_KERNEL); - if (!lw) - return false; - - INIT_WORK(&lw->lease_break, cifs_ses_oplock_break); - lw->lease_state = rsp->NewLeaseState; cifs_dbg(FYI, "Checking for lease break\n"); @@ -600,15 +612,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 +633,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+ --- fs/cifs/smb2misc.c | 47 ++++++++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 20 deletions(-)