Re: [cifs:for-next 31/31] fs/cifs/smb2ops.c:786 open_shroot() error: double unlock 'mutex:&tcon->crfid.fid_mutex'

[Steve French <smfrench@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4597 bytes --]

Updated patch, addressing the problem Dan pointed out is attached.

On Fri, Sep 13, 2019 at 8:58 AM Dan Carpenter via samba-technical
<samba-technical@lists.samba.org> wrote:
>
> tree:   git://git.samba.org/sfrench/cifs-2.6.git for-next
> head:   5fc321fb644fc787710353be11129edadd313f3a
> commit: 5fc321fb644fc787710353be11129edadd313f3a [31/31] smb3: fix unmount hang in open_shroot
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> New smatch warnings:
> fs/cifs/smb2ops.c:786 open_shroot() error: double unlock 'mutex:&tcon->crfid.fid_mutex'
>
> git remote add cifs git://git.samba.org/sfrench/cifs-2.6.git
> git remote update cifs
> git checkout 5fc321fb644fc787710353be11129edadd313f3a
> vim +786 fs/cifs/smb2ops.c
>
> fs/cifs/smb2ops.c
>    726                  /*
>    727                   * caller expects this func to set pfid to a valid
>    728                   * cached root, so we copy the existing one and get a
>    729                   * reference.
>    730                   */
>    731                  memcpy(pfid, tcon->crfid.fid, sizeof(*pfid));
>    732                  kref_get(&tcon->crfid.refcount);
>    733
>    734                  mutex_unlock(&tcon->crfid.fid_mutex);
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Unlock (recently added)
>
>    735
>    736                  if (rc == 0) {
>    737                          /* close extra handle outside of crit sec */
>    738                          SMB2_close(xid, tcon, fid.persistent_fid, fid.volatile_fid);
>    739                  }
>    740                  goto oshr_free;
>    741          }
>    742
>    743          /* Cached root is still invalid, continue normaly */
>    744
>    745          if (rc) {
>    746                  if (rc == -EREMCHG) {
>    747                          tcon->need_reconnect = true;
>    748                          printk_once(KERN_WARNING "server share %s deleted\n",
>    749                                      tcon->treeName);
>    750                  }
>    751                  goto oshr_exit;
>    752          }
>    753
>    754          o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base;
>    755          oparms.fid->persistent_fid = o_rsp->PersistentFileId;
>    756          oparms.fid->volatile_fid = o_rsp->VolatileFileId;
>    757  #ifdef CONFIG_CIFS_DEBUG2
>    758          oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId);
>    759  #endif /* CIFS_DEBUG2 */
>    760
>    761          memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
>    762          tcon->crfid.tcon = tcon;
>    763          tcon->crfid.is_valid = true;
>    764          kref_init(&tcon->crfid.refcount);
>    765
>    766          /* BB TBD check to see if oplock level check can be removed below */
>    767          if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) {
>    768                  kref_get(&tcon->crfid.refcount);
>    769                  smb2_parse_contexts(server, o_rsp,
>    770                                  &oparms.fid->epoch,
>    771                                  oparms.fid->lease_key, &oplock, NULL);
>    772          } else
>    773                  goto oshr_exit;
>    774
>    775          qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
>    776          if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info))
>    777                  goto oshr_exit;
>    778          if (!smb2_validate_and_copy_iov(
>    779                                  le16_to_cpu(qi_rsp->OutputBufferOffset),
>    780                                  sizeof(struct smb2_file_all_info),
>    781                                  &rsp_iov[1], sizeof(struct smb2_file_all_info),
>    782                                  (char *)&tcon->crfid.file_all_info))
>    783                  tcon->crfid.file_all_info_is_valid = 1;
>    784
>    785  oshr_exit:
>    786          mutex_unlock(&tcon->crfid.fid_mutex);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Double unlock.
>
>    787  oshr_free:
>    788          SMB2_open_free(&rqst[0]);
>    789          SMB2_query_info_free(&rqst[1]);
>    790          free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
>    791          free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
>    792          return rc;
>    793  }
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
>


-- 
Thanks,

Steve

[-- Attachment #2: 0001-smb3-fix-unmount-hang-in-open_shroot.patch --]
[-- Type: text/x-patch, Size: 2765 bytes --]

From 34d8dc26eb1f37040b4819e65dfde4017ae3a281 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Thu, 12 Sep 2019 17:52:54 -0500
Subject: [PATCH] smb3: fix unmount hang in open_shroot

An earlier patch "CIFS: fix deadlock in cached root handling"
did not completely address the deadlock in open_shroot. This
patch addresses the deadlock.

In testing the recent patch:
  smb3: improve handling of share deleted (and share recreated)
we were able to reproduce the open_shroot deadlock to one
of the target servers in unmount in a delete share scenario.

Fixes: 7e5a70ad88b1e ("CIFS: fix deadlock in cached root handling")

This is version 2 of this patch. An earlier version of this
patch "smb3: fix unmount hang in open_shroot" had a problem
found by Dan.

Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

Suggested-by: Pavel Shilovsky <pshilov@microsoft.com>
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
CC: Aurelien Aptel <aaptel@suse.com>
CC: Stable <stable@vger.kernel.org>
---
 fs/cifs/smb2ops.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 3672ce0bfbaf..5776d7b0a97e 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -658,6 +658,15 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
 		return 0;
 	}
 
+	/*
+	 * We do not hold the lock for the open because in case
+	 * SMB2_open needs to reconnect, it will end up calling
+	 * cifs_mark_open_files_invalid() which takes the lock again
+	 * thus causing a deadlock
+	 */
+
+	mutex_unlock(&tcon->crfid.fid_mutex);
+
 	if (smb3_encryption_required(tcon))
 		flags |= CIFS_TRANSFORM_REQ;
 
@@ -679,7 +688,7 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
 
 	rc = SMB2_open_init(tcon, &rqst[0], &oplock, &oparms, &utf16_path);
 	if (rc)
-		goto oshr_exit;
+		goto oshr_free;
 	smb2_set_next_command(tcon, &rqst[0]);
 
 	memset(&qi_iov, 0, sizeof(qi_iov));
@@ -692,18 +701,10 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
 				  sizeof(struct smb2_file_all_info) +
 				  PATH_MAX * 2, 0, NULL);
 	if (rc)
-		goto oshr_exit;
+		goto oshr_free;
 
 	smb2_set_related(&rqst[1]);
 
-	/*
-	 * We do not hold the lock for the open because in case
-	 * SMB2_open needs to reconnect, it will end up calling
-	 * cifs_mark_open_files_invalid() which takes the lock again
-	 * thus causing a deadlock
-	 */
-
-	mutex_unlock(&tcon->crfid.fid_mutex);
 	rc = compound_send_recv(xid, ses, flags, 2, rqst,
 				resp_buftype, rsp_iov);
 	mutex_lock(&tcon->crfid.fid_mutex);
-- 
2.20.1