diff mbox series

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

Message ID 20200710050116.623540-1-paul@darkrain42.org (mailing list archive)
State New, archived
Headers show
Series [v4] cifs: Fix leak when handling lease break for cached root fid | expand

Commit Message

Paul Aurich July 10, 2020, 5:01 a.m. UTC 
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 from v3:
  - refactor pending open lease handling into separate functions so that the
    core spinlock handling can occur all in smb2_is_valid_lease_break

 fs/cifs/smb2misc.c | 73 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 52 insertions(+), 21 deletions(-)

Comments

Steve French July 11, 2020, 4:41 p.m. UTC | #1 
tentatively merged into cifs-2.6.git for-next pending review and testing

On Fri, Jul 10, 2020 at 12:01 AM 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+
> ---
> Changes from v3:
>   - refactor pending open lease handling into separate functions so that the
>     core spinlock handling can occur all in smb2_is_valid_lease_break
>
>  fs/cifs/smb2misc.c | 73 +++++++++++++++++++++++++++++++++-------------
>  1 file changed, 52 insertions(+), 21 deletions(-)
>
> diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
> index 6a39451973f8..865cd248c605 100644
> --- a/fs/cifs/smb2misc.c
> +++ b/fs/cifs/smb2misc.c
> @@ -504,15 +504,31 @@ cifs_ses_oplock_break(struct work_struct *work)
>         kfree(lw);
>  }
>
> +static void
> +smb2_queue_pending_open_break(struct tcon_link *tlink, __u8 *lease_key,
> +                             __le32 new_lease_state)
> +{
> +       struct smb2_lease_break_work *lw;
> +
> +       lw = kmalloc(sizeof(struct smb2_lease_break_work), GFP_KERNEL);
> +       if (!lw) {
> +               cifs_put_tlink(tlink);
> +               return;
> +       }
> +
> +       INIT_WORK(&lw->lease_break, cifs_ses_oplock_break);
> +       lw->tlink = tlink;
> +       lw->lease_state = new_lease_state;
> +       memcpy(lw->lease_key, lease_key, SMB2_LEASE_KEY_SIZE);
> +       queue_work(cifsiod_wq, &lw->lease_break);
> +}
> +
>  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;
>         struct list_head *tmp;
>         struct cifsFileInfo *cfile;
> -       struct cifs_pending_open *open;
>         struct cifsInodeInfo *cinode;
>         int ack_req = le32_to_cpu(rsp->Flags &
>                                   SMB2_NOTIFY_BREAK_LEASE_FLAG_ACK_REQUIRED);
> @@ -542,22 +558,29 @@ 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);
>                 return true;
>         }
>
> -       found = false;
> +       return false;
> +}
> +
> +static struct cifs_pending_open *
> +smb2_tcon_find_pending_open_lease(struct cifs_tcon *tcon,
> +                                 struct smb2_lease_break *rsp)
> +{
> +       __u8 lease_state = le32_to_cpu(rsp->NewLeaseState);
> +       int ack_req = le32_to_cpu(rsp->Flags &
> +                                 SMB2_NOTIFY_BREAK_LEASE_FLAG_ACK_REQUIRED);
> +       struct cifs_pending_open *open;
> +       struct cifs_pending_open *found = NULL;
> +
>         list_for_each_entry(open, &tcon->pending_opens, olist) {
>                 if (memcmp(open->lease_key, rsp->LeaseKey,
>                            SMB2_LEASE_KEY_SIZE))
>                         continue;
>
>                 if (!found && ack_req) {
> -                       found = true;
> -                       memcpy(lw->lease_key, open->lease_key,
> -                              SMB2_LEASE_KEY_SIZE);
> -                       lw->tlink = cifs_get_tlink(open->tlink);
> -                       queue_work(cifsiod_wq, &lw->lease_break);
> +                       found = open;
>                 }
>
>                 cifs_dbg(FYI, "found in the pending open list\n");
> @@ -578,14 +601,7 @@ 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;
> +       struct cifs_pending_open *open;
>
>         cifs_dbg(FYI, "Checking for lease break\n");
>
> @@ -603,11 +619,27 @@ smb2_is_valid_lease_break(char *buffer)
>                                 spin_lock(&tcon->open_file_lock);
>                                 cifs_stats_inc(
>                                     &tcon->stats.cifs_stats.num_oplock_brks);
> -                               if (smb2_tcon_has_lease(tcon, rsp, lw)) {
> +                               if (smb2_tcon_has_lease(tcon, rsp)) {
>                                         spin_unlock(&tcon->open_file_lock);
>                                         spin_unlock(&cifs_tcp_ses_lock);
>                                         return true;
>                                 }
> +                               open = smb2_tcon_find_pending_open_lease(tcon,
> +                                                                        rsp);
> +                               if (open) {
> +                                       __u8 lease_key[SMB2_LEASE_KEY_SIZE];
> +                                       struct tcon_link *tlink;
> +
> +                                       tlink = cifs_get_tlink(open->tlink);
> +                                       memcpy(lease_key, open->lease_key,
> +                                              SMB2_LEASE_KEY_SIZE);
> +                                       spin_unlock(&tcon->open_file_lock);
> +                                       spin_unlock(&cifs_tcp_ses_lock);
> +                                       smb2_queue_pending_open_break(tlink,
> +                                                                     lease_key,
> +                                                                     rsp->NewLeaseState);
> +                                       return true;
> +                               }
>                                 spin_unlock(&tcon->open_file_lock);
>
>                                 if (tcon->crfid.is_valid &&
> @@ -625,7 +657,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
>
Aurélien Aptel July 29, 2020, 2:02 p.m. UTC | #2 
Sorry for the delay...

Reviewed-by: Aurelien Aptel <aaptel@suse.com>
diff mbox series

Patch

diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
index 6a39451973f8..865cd248c605 100644
--- a/fs/cifs/smb2misc.c
+++ b/fs/cifs/smb2misc.c
@@ -504,15 +504,31 @@  cifs_ses_oplock_break(struct work_struct *work)
 	kfree(lw);
 }
 
+static void
+smb2_queue_pending_open_break(struct tcon_link *tlink, __u8 *lease_key,
+			      __le32 new_lease_state)
+{
+	struct smb2_lease_break_work *lw;
+
+	lw = kmalloc(sizeof(struct smb2_lease_break_work), GFP_KERNEL);
+	if (!lw) {
+		cifs_put_tlink(tlink);
+		return;
+	}
+
+	INIT_WORK(&lw->lease_break, cifs_ses_oplock_break);
+	lw->tlink = tlink;
+	lw->lease_state = new_lease_state;
+	memcpy(lw->lease_key, lease_key, SMB2_LEASE_KEY_SIZE);
+	queue_work(cifsiod_wq, &lw->lease_break);
+}
+
 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;
 	struct list_head *tmp;
 	struct cifsFileInfo *cfile;
-	struct cifs_pending_open *open;
 	struct cifsInodeInfo *cinode;
 	int ack_req = le32_to_cpu(rsp->Flags &
 				  SMB2_NOTIFY_BREAK_LEASE_FLAG_ACK_REQUIRED);
@@ -542,22 +558,29 @@  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);
 		return true;
 	}
 
-	found = false;
+	return false;
+}
+
+static struct cifs_pending_open *
+smb2_tcon_find_pending_open_lease(struct cifs_tcon *tcon,
+				  struct smb2_lease_break *rsp)
+{
+	__u8 lease_state = le32_to_cpu(rsp->NewLeaseState);
+	int ack_req = le32_to_cpu(rsp->Flags &
+				  SMB2_NOTIFY_BREAK_LEASE_FLAG_ACK_REQUIRED);
+	struct cifs_pending_open *open;
+	struct cifs_pending_open *found = NULL;
+
 	list_for_each_entry(open, &tcon->pending_opens, olist) {
 		if (memcmp(open->lease_key, rsp->LeaseKey,
 			   SMB2_LEASE_KEY_SIZE))
 			continue;
 
 		if (!found && ack_req) {
-			found = true;
-			memcpy(lw->lease_key, open->lease_key,
-			       SMB2_LEASE_KEY_SIZE);
-			lw->tlink = cifs_get_tlink(open->tlink);
-			queue_work(cifsiod_wq, &lw->lease_break);
+			found = open;
 		}
 
 		cifs_dbg(FYI, "found in the pending open list\n");
@@ -578,14 +601,7 @@  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;
+	struct cifs_pending_open *open;
 
 	cifs_dbg(FYI, "Checking for lease break\n");
 
@@ -603,11 +619,27 @@  smb2_is_valid_lease_break(char *buffer)
 				spin_lock(&tcon->open_file_lock);
 				cifs_stats_inc(
 				    &tcon->stats.cifs_stats.num_oplock_brks);
-				if (smb2_tcon_has_lease(tcon, rsp, lw)) {
+				if (smb2_tcon_has_lease(tcon, rsp)) {
 					spin_unlock(&tcon->open_file_lock);
 					spin_unlock(&cifs_tcp_ses_lock);
 					return true;
 				}
+				open = smb2_tcon_find_pending_open_lease(tcon,
+									 rsp);
+				if (open) {
+					__u8 lease_key[SMB2_LEASE_KEY_SIZE];
+					struct tcon_link *tlink;
+
+					tlink = cifs_get_tlink(open->tlink);
+					memcpy(lease_key, open->lease_key,
+					       SMB2_LEASE_KEY_SIZE);
+					spin_unlock(&tcon->open_file_lock);
+					spin_unlock(&cifs_tcp_ses_lock);
+					smb2_queue_pending_open_break(tlink,
+								      lease_key,
+								      rsp->NewLeaseState);
+					return true;
+				}
 				spin_unlock(&tcon->open_file_lock);
 
 				if (tcon->crfid.is_valid &&
@@ -625,7 +657,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;
 }