diff mbox series

smb3: fix unusable share after force unmount failure

Message ID CAH2r5mvJvJdo61-qQ6GjCw_NBL7ZM6dx-9SaacaKpRb0rgGpFQ@mail.gmail.com (mailing list archive)
State New, archived
Headers show
Series smb3: fix unusable share after force unmount failure | expand

Commit Message

Steve French March 23, 2023, 9:30 p.m. UTC
If user does forced unmount ("umount -f") while files are still open
on the share (as was seen in a Kubernetes example running on SMB3.1.1
mount) then we were marking the share as "TID_EXITING" in umount_begin()
which caused all subsequent operations (except write) to fail ... but
unfortunately when umount_begin() is called we do not know yet that
there are open files or active references on the share that would prevent
unmount from succeeding.  Kubernetes had example when they were doing
umount -f when files were open which caused the share to become
unusable until the files were closed (and the umount retried).

Fix this so that TID_EXITING is not set until we are about to send
the tree disconnect (not at the beginning of forced umounts in
umount_begin) so that if "umount -f" fails (due to open files or
references) the mount is still usable.

Cc: stable@vger.kernel.org
Signed-off-by: Steve French <stfrench@microsoft.com>

Comments

Steve French March 24, 2023, 5:52 p.m. UTC | #1
updated based on Paulo's feedback (moved the TID_EXITING up so it is
inside the tc_locktc_unlock).  See attached.




On Thu, Mar 23, 2023 at 4:30 PM Steve French <smfrench@gmail.com> wrote:
>
> If user does forced unmount ("umount -f") while files are still open
> on the share (as was seen in a Kubernetes example running on SMB3.1.1
> mount) then we were marking the share as "TID_EXITING" in umount_begin()
> which caused all subsequent operations (except write) to fail ... but
> unfortunately when umount_begin() is called we do not know yet that
> there are open files or active references on the share that would prevent
> unmount from succeeding.  Kubernetes had example when they were doing
> umount -f when files were open which caused the share to become
> unusable until the files were closed (and the umount retried).
>
> Fix this so that TID_EXITING is not set until we are about to send
> the tree disconnect (not at the beginning of forced umounts in
> umount_begin) so that if "umount -f" fails (due to open files or
> references) the mount is still usable.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Steve French <stfrench@microsoft.com>
>
>
>
> --
> Thanks,
>
> Steve
diff mbox series

Patch

From ed55df0da747abdc1db75fef8baf3b0cac2c2de7 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Thu, 23 Mar 2023 16:20:02 -0500
Subject: [PATCH] smb3: fix unusable share after force unmount failure

If user does forced unmount ("umount -f") while files are still open
on the share (as was seen in a Kubernetes example running on SMB3.1.1
mount) then we were marking the share as "TID_EXITING" in umount_begin()
which caused all subsequent operations (except write) to fail ... but
unfortunately when umount_begin() is called we do not know yet that
there are open files or active references on the share that would prevent
unmount from succeeding.  Kubernetes had example when they were doing
umount -f when files were open which caused the share to become
unusable until the files were closed (and the umount retried).

Fix this so that TID_EXITING is not set until we are about to send
the tree disconnect (not at the beginning of forced umounts in
umount_begin) so that if "umount -f" fails (due to open files or
references) the mount is still usable.

Cc: stable@vger.kernel.org
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/cifsfs.c  | 9 ++++++---
 fs/cifs/cifssmb.c | 6 ++----
 fs/cifs/connect.c | 4 +++-
 fs/cifs/smb2pdu.c | 8 ++------
 4 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index cbcf210d56e4..ac9034fce409 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -731,13 +731,16 @@  static void cifs_umount_begin(struct super_block *sb)
 	spin_lock(&tcon->tc_lock);
 	if ((tcon->tc_count > 1) || (tcon->status == TID_EXITING)) {
 		/* we have other mounts to same share or we have
-		   already tried to force umount this and woken up
+		   already tried to umount this and woken up
 		   all waiting network requests, nothing to do */
 		spin_unlock(&tcon->tc_lock);
 		spin_unlock(&cifs_tcp_ses_lock);
 		return;
-	} else if (tcon->tc_count == 1)
-		tcon->status = TID_EXITING;
+	}
+	/*
+	 * can not set tcon->status to TID_EXITING yet since we don't know if umount -f will
+	 * fail later (e.g. due to open files).  TID_EXITING will be set just before tdis req sent
+	 */
 	spin_unlock(&tcon->tc_lock);
 	spin_unlock(&cifs_tcp_ses_lock);
 
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index a43c78396dd8..38a697eca305 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -86,13 +86,11 @@  cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
 
 	/*
 	 * only tree disconnect, open, and write, (and ulogoff which does not
-	 * have tcon) are allowed as we start force umount
+	 * have tcon) are allowed as we start umount
 	 */
 	spin_lock(&tcon->tc_lock);
 	if (tcon->status == TID_EXITING) {
-		if (smb_command != SMB_COM_WRITE_ANDX &&
-		    smb_command != SMB_COM_OPEN_ANDX &&
-		    smb_command != SMB_COM_TREE_DISCONNECT) {
+		if (smb_command != SMB_COM_TREE_DISCONNECT) {
 			spin_unlock(&tcon->tc_lock);
 			cifs_dbg(FYI, "can not send cmd %d while umounting\n",
 				 smb_command);
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index c3162ef9c9e9..4c3d4d4002b9 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2341,8 +2341,10 @@  cifs_put_tcon(struct cifs_tcon *tcon)
 	}
 
 	xid = get_xid();
-	if (ses->server->ops->tree_disconnect)
+	if (ses->server->ops->tree_disconnect) {
+		tcon->status = TID_EXITING;
 		ses->server->ops->tree_disconnect(xid, tcon);
+	}
 	_free_xid(xid);
 
 	cifs_fscache_release_super_cookie(tcon);
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index a9fb95b7ef82..41b9f41a7505 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -165,13 +165,9 @@  smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
 	spin_lock(&tcon->tc_lock);
 	if (tcon->status == TID_EXITING) {
 		/*
-		 * only tree disconnect, open, and write,
-		 * (and ulogoff which does not have tcon)
-		 * are allowed as we start force umount.
+		 * only tree disconnect allowed when disconnecting ...
 		 */
-		if ((smb2_command != SMB2_WRITE) &&
-		   (smb2_command != SMB2_CREATE) &&
-		   (smb2_command != SMB2_TREE_DISCONNECT)) {
+		if (smb2_command != SMB2_TREE_DISCONNECT) {
 			spin_unlock(&tcon->tc_lock);
 			cifs_dbg(FYI, "can not send cmd %d while umounting\n",
 				 smb2_command);
-- 
2.34.1