[PATCH v2 0/5] Stable fixes

[PATCH v2 0/5] Stable fixes

[Pavel Shilovsky]

Here are a bunch of fixes targeted for stable. The first tree patches
are small fixes. The patch #4 fixes the kernel crash due to a memory
corruption bug during reconnect. The patch #5 fixes a possible stucking
on the mutex which was already locked by the same process.

Changes since v1:
 - Addressed Sachin's suggestions about replacing mod_delayed_work with
queue_delayed_work in patches #4 and #5.
 - Updated a description of patch #5.

Pavel Shilovsky (5):
  CIFS: Decrease verbosity of ioctl call
  CIFS: Fix missing nls unload in smb2_reconnect()
  CIFS: Fix a possible memory corruption in push locks
  CIFS: Fix a possible memory corruption during reconnect
  CIFS: Fix a possible double locking of mutex during reconnect

 fs/cifs/cifsglob.h  |  4 +++
 fs/cifs/cifsproto.h |  3 ++
 fs/cifs/connect.c   | 34 ++++++++++++++++-----
 fs/cifs/file.c      |  8 ++++-
 fs/cifs/ioctl.c     |  2 +-
 fs/cifs/smb2file.c  |  2 +-
 fs/cifs/smb2pdu.c   | 87 ++++++++++++++++++++++++++++++++++++-----------------
 fs/cifs/smb2pdu.h   |  2 ++
 fs/cifs/smb2proto.h |  1 +
 9 files changed, 105 insertions(+), 38 deletions(-)

-- 
2.7.4

[PATCH v2 1/5] CIFS: Decrease verbosity of ioctl call

[Pavel Shilovsky]

Cc: Stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> # v4.9+
Reviewed-by: Aurelien Aptel <aaptel-IBi9RG/b67k@public.gmane.org>
Acked-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Pavel Shilovsky <pshilov-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>
---
 fs/cifs/ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/ioctl.c b/fs/cifs/ioctl.c
index 9f51b81..0015287 100644
--- a/fs/cifs/ioctl.c
+++ b/fs/cifs/ioctl.c
@@ -189,7 +189,7 @@ long cifs_ioctl(struct file *filep, unsigned int command, unsigned long arg)
 	xid = get_xid();
 
 	cifs_sb = CIFS_SB(inode->i_sb);
-	cifs_dbg(VFS, "cifs ioctl 0x%x\n", command);
+	cifs_dbg(FYI, "cifs ioctl 0x%x\n", command);
 	switch (command) {
 		case FS_IOC_GETFLAGS:
 			if (pSMBFile == NULL)
-- 
2.7.4

[PATCH v2 2/5] CIFS: Fix missing nls unload in smb2_reconnect()

[Pavel Shilovsky]

Cc: Stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Acked-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Pavel Shilovsky <pshilov-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>
---
 fs/cifs/smb2pdu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 5ca5ea46..730d57b 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -280,7 +280,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon)
 	case SMB2_CHANGE_NOTIFY:
 	case SMB2_QUERY_INFO:
 	case SMB2_SET_INFO:
-		return -EAGAIN;
+		rc = -EAGAIN;
 	}
 	unload_nls(nls_codepage);
 	return rc;
-- 
2.7.4

[PATCH v2 3/5] CIFS: Fix a possible memory corruption in push locks

[Pavel Shilovsky]

If maxBuf is not 0 but less than a size of SMB2 lock structure
we can end up with a memory corruption.

Cc: Stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Signed-off-by: Pavel Shilovsky <pshilov-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>
---
 fs/cifs/smb2file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/smb2file.c b/fs/cifs/smb2file.c
index f9e766f..b2aff0c 100644
--- a/fs/cifs/smb2file.c
+++ b/fs/cifs/smb2file.c
@@ -260,7 +260,7 @@ smb2_push_mandatory_locks(struct cifsFileInfo *cfile)
 	 * and check it for zero before using.
 	 */
 	max_buf = tlink_tcon(cfile->tlink)->ses->server->maxBuf;
-	if (!max_buf) {
+	if (max_buf < sizeof(struct smb2_lock_element)) {
 		free_xid(xid);
 		return -EINVAL;
 	}
-- 
2.7.4

[PATCH v2 4/5] CIFS: Fix a possible memory corruption during reconnect

[Pavel Shilovsky]

We can not unlock/lock cifs_tcp_ses_lock while walking through ses
and tcon lists because it can corrupt list iterator pointers and
a tcon structure can be released if we don't hold an extra reference.
Fix it by moving a reconnect process to a separate delayed work
and acquiring a reference to every tcon that needs to be reconnected.
Also do not send an echo request on newly established connections.

CC: Stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Signed-off-by: Pavel Shilovsky <pshilov-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>
---
 fs/cifs/cifsglob.h  |  3 +++
 fs/cifs/cifsproto.h |  3 +++
 fs/cifs/connect.c   | 34 +++++++++++++++++++-----
 fs/cifs/smb2pdu.c   | 75 ++++++++++++++++++++++++++++++++++++-----------------
 fs/cifs/smb2proto.h |  1 +
 5 files changed, 85 insertions(+), 31 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 3e95191..89a0d7f 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -647,6 +647,8 @@ struct TCP_Server_Info {
 	unsigned int	max_read;
 	unsigned int	max_write;
 	__u8		preauth_hash[512];
+	struct delayed_work reconnect; /* reconnect workqueue job */
+	struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
 #endif /* CONFIG_CIFS_SMB2 */
 	unsigned long echo_interval;
 };
@@ -850,6 +852,7 @@ cap_unix(struct cifs_ses *ses)
 struct cifs_tcon {
 	struct list_head tcon_list;
 	int tc_count;
+	struct list_head rlist; /* reconnect list */
 	struct list_head openFileList;
 	spinlock_t open_file_lock; /* protects list above */
 	struct cifs_ses *ses;	/* pointer to session associated with */
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index ced0e42..cd8025a 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -206,6 +206,9 @@ extern void cifs_add_pending_open_locked(struct cifs_fid *fid,
 					 struct tcon_link *tlink,
 					 struct cifs_pending_open *open);
 extern void cifs_del_pending_open(struct cifs_pending_open *open);
+extern void cifs_put_tcp_session(struct TCP_Server_Info *server,
+				 int from_reconnect);
+extern void cifs_put_tcon(struct cifs_tcon *tcon);
 
 #if IS_ENABLED(CONFIG_CIFS_DFS_UPCALL)
 extern void cifs_dfs_release_automount_timer(void);
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 5563de3..5aaf7b1 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -52,6 +52,9 @@
 #include "nterr.h"
 #include "rfc1002pdu.h"
 #include "fscache.h"
+#ifdef CONFIG_CIFS_SMB2
+#include "smb2proto.h"
+#endif
 
 #define CIFS_PORT 445
 #define RFC1001_PORT 139
@@ -2110,8 +2113,8 @@ cifs_find_tcp_session(struct smb_vol *vol)
 	return NULL;
 }
 
-static void
-cifs_put_tcp_session(struct TCP_Server_Info *server)
+void
+cifs_put_tcp_session(struct TCP_Server_Info *server, int from_reconnect)
 {
 	struct task_struct *task;
 
@@ -2128,6 +2131,19 @@ cifs_put_tcp_session(struct TCP_Server_Info *server)
 
 	cancel_delayed_work_sync(&server->echo);
 
+#ifdef CONFIG_CIFS_SMB2
+	if (from_reconnect)
+		/*
+		 * Avoid deadlock here: reconnect work calls
+		 * cifs_put_tcp_session() at its end. Need to be sure
+		 * that reconnect work does nothing with server pointer after
+		 * that step.
+		 */
+		cancel_delayed_work(&server->reconnect);
+	else
+		cancel_delayed_work_sync(&server->reconnect);
+#endif
+
 	spin_lock(&GlobalMid_Lock);
 	server->tcpStatus = CifsExiting;
 	spin_unlock(&GlobalMid_Lock);
@@ -2192,6 +2208,10 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 	INIT_LIST_HEAD(&tcp_ses->tcp_ses_list);
 	INIT_LIST_HEAD(&tcp_ses->smb_ses_list);
 	INIT_DELAYED_WORK(&tcp_ses->echo, cifs_echo_request);
+#ifdef CONFIG_CIFS_SMB2
+	INIT_DELAYED_WORK(&tcp_ses->reconnect, smb2_reconnect_server);
+	mutex_init(&tcp_ses->reconnect_mutex);
+#endif
 	memcpy(&tcp_ses->srcaddr, &volume_info->srcaddr,
 	       sizeof(tcp_ses->srcaddr));
 	memcpy(&tcp_ses->dstaddr, &volume_info->dstaddr,
@@ -2350,7 +2370,7 @@ cifs_put_smb_ses(struct cifs_ses *ses)
 	spin_unlock(&cifs_tcp_ses_lock);
 
 	sesInfoFree(ses);
-	cifs_put_tcp_session(server);
+	cifs_put_tcp_session(server, 0);
 }
 
 #ifdef CONFIG_KEYS
@@ -2524,7 +2544,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
 		mutex_unlock(&ses->session_mutex);
 
 		/* existing SMB ses has a server reference already */
-		cifs_put_tcp_session(server);
+		cifs_put_tcp_session(server, 0);
 		free_xid(xid);
 		return ses;
 	}
@@ -2620,7 +2640,7 @@ cifs_find_tcon(struct cifs_ses *ses, struct smb_vol *volume_info)
 	return NULL;
 }
 
-static void
+void
 cifs_put_tcon(struct cifs_tcon *tcon)
 {
 	unsigned int xid;
@@ -3824,7 +3844,7 @@ cifs_mount(struct cifs_sb_info *cifs_sb, struct smb_vol *volume_info)
 		else if (ses)
 			cifs_put_smb_ses(ses);
 		else
-			cifs_put_tcp_session(server);
+			cifs_put_tcp_session(server, 0);
 		bdi_destroy(&cifs_sb->bdi);
 	}
 
@@ -4135,7 +4155,7 @@ cifs_construct_tcon(struct cifs_sb_info *cifs_sb, kuid_t fsuid)
 	ses = cifs_get_smb_ses(master_tcon->ses->server, vol_info);
 	if (IS_ERR(ses)) {
 		tcon = (struct cifs_tcon *)ses;
-		cifs_put_tcp_session(master_tcon->ses->server);
+		cifs_put_tcp_session(master_tcon->ses->server, 0);
 		goto out;
 	}
 
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 730d57b..4ba3f68 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -1972,6 +1972,54 @@ smb2_echo_callback(struct mid_q_entry *mid)
 	add_credits(server, credits_received, CIFS_ECHO_OP);
 }
 
+void smb2_reconnect_server(struct work_struct *work)
+{
+	struct TCP_Server_Info *server = container_of(work,
+					struct TCP_Server_Info, reconnect.work);
+	struct cifs_ses *ses;
+	struct cifs_tcon *tcon, *tcon2;
+	struct list_head tmp_list;
+	int tcon_exist = false;
+
+	/* Prevent simultaneous reconnects that can corrupt tcon->rlist list */
+	mutex_lock(&server->reconnect_mutex);
+
+	INIT_LIST_HEAD(&tmp_list);
+	cifs_dbg(FYI, "Need negotiate, reconnecting tcons\n");
+
+	spin_lock(&cifs_tcp_ses_lock);
+	list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) {
+		list_for_each_entry(tcon, &ses->tcon_list, tcon_list) {
+			if (tcon->need_reconnect) {
+				tcon->tc_count++;
+				list_add_tail(&tcon->rlist, &tmp_list);
+				tcon_exist = true;
+			}
+		}
+	}
+	/*
+	 * Get the reference to server struct to be sure that the last call of
+	 * cifs_put_tcon() in the loop below won't release the server pointer.
+	 */
+	if (tcon_exist)
+		server->srv_count++;
+
+	spin_unlock(&cifs_tcp_ses_lock);
+
+	list_for_each_entry_safe(tcon, tcon2, &tmp_list, rlist) {
+		smb2_reconnect(SMB2_ECHO, tcon);
+		list_del_init(&tcon->rlist);
+		cifs_put_tcon(tcon);
+	}
+
+	cifs_dbg(FYI, "Reconnecting tcons finished\n");
+	mutex_unlock(&server->reconnect_mutex);
+
+	/* now we can safely release srv struct */
+	if (tcon_exist)
+		cifs_put_tcp_session(server, 1);
+}
+
 int
 SMB2_echo(struct TCP_Server_Info *server)
 {
@@ -1984,32 +2032,11 @@ SMB2_echo(struct TCP_Server_Info *server)
 	cifs_dbg(FYI, "In echo request\n");
 
 	if (server->tcpStatus == CifsNeedNegotiate) {
-		struct list_head *tmp, *tmp2;
-		struct cifs_ses *ses;
-		struct cifs_tcon *tcon;
-
-		cifs_dbg(FYI, "Need negotiate, reconnecting tcons\n");
-		spin_lock(&cifs_tcp_ses_lock);
-		list_for_each(tmp, &server->smb_ses_list) {
-			ses = list_entry(tmp, struct cifs_ses, smb_ses_list);
-			list_for_each(tmp2, &ses->tcon_list) {
-				tcon = list_entry(tmp2, struct cifs_tcon,
-						  tcon_list);
-				/* add check for persistent handle reconnect */
-				if (tcon && tcon->need_reconnect) {
-					spin_unlock(&cifs_tcp_ses_lock);
-					rc = smb2_reconnect(SMB2_ECHO, tcon);
-					spin_lock(&cifs_tcp_ses_lock);
-				}
-			}
-		}
-		spin_unlock(&cifs_tcp_ses_lock);
+		/* No need to send echo on newly established connections */
+		queue_delayed_work(cifsiod_wq, &server->reconnect, 0);
+		return rc;
 	}
 
-	/* if no session, renegotiate failed above */
-	if (server->tcpStatus == CifsNeedNegotiate)
-		return -EIO;
-
 	rc = small_smb2_init(SMB2_ECHO, NULL, (void **)&req);
 	if (rc)
 		return rc;
diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
index eb2cde2..f2d511a 100644
--- a/fs/cifs/smb2proto.h
+++ b/fs/cifs/smb2proto.h
@@ -96,6 +96,7 @@ extern int smb2_open_file(const unsigned int xid,
 extern int smb2_unlock_range(struct cifsFileInfo *cfile,
 			     struct file_lock *flock, const unsigned int xid);
 extern int smb2_push_mandatory_locks(struct cifsFileInfo *cfile);
+extern void smb2_reconnect_server(struct work_struct *work);
 
 /*
  * SMB2 Worker functions - most of protocol specific implementation details
-- 
2.7.4

[PATCH v2 5/5] CIFS: Fix a possible double locking of mutex during reconnect

[Pavel Shilovsky]

With the current code it is possible to lock a mutex twice when
a subsequent reconnects are triggered. On the 1st reconnect we
reconnect sessions and tcons and then persistent file handles.
If the 2nd reconnect happens during the reconnecting of persistent
file handles then the following sequence of calls is observed:

cifs_reopen_file -> SMB2_open -> small_smb2_init -> smb2_reconnect
-> cifs_reopen_persistent_file_handles -> cifs_reopen_file (again!).

So, we are trying to acquire the same cfile->fh_mutex twice which
is wrong. Fix this by moving reconnecting of persistent handles to
the delayed work (smb2_reconnect_server) and submitting this work
every time we reconnect tcon in SMB2 commands handling codepath.

This can also lead to corruption of a temporary file list in
cifs_reopen_persistent_file_handles() because we can recursively
call this function twice.

Cc: Stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> # v4.9+
Signed-off-by: Pavel Shilovsky <pshilov-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>
---
 fs/cifs/cifsglob.h |  1 +
 fs/cifs/file.c     |  8 +++++++-
 fs/cifs/smb2pdu.c  | 14 +++++++++-----
 fs/cifs/smb2pdu.h  |  2 ++
 4 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 89a0d7f..59ab5a2 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -926,6 +926,7 @@ struct cifs_tcon {
 	bool broken_posix_open; /* e.g. Samba server versions < 3.3.2, 3.2.9 */
 	bool broken_sparse_sup; /* if server or share does not support sparse */
 	bool need_reconnect:1; /* connection reset, tid now invalid */
+	bool need_reopen_files:1; /* need to reopen tcon file handles */
 	bool use_resilient:1; /* use resilient instead of durable handles */
 	bool use_persistent:1; /* use persistent instead of durable handles */
 #ifdef CONFIG_CIFS_SMB2
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 7f5f617..18a1e1d 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -777,6 +777,11 @@ cifs_reopen_persistent_handles(struct cifs_tcon *tcon)
 	struct list_head *tmp1;
 	struct list_head tmp_list;
 
+	if (!tcon->use_persistent || !tcon->need_reopen_files)
+		return;
+
+	tcon->need_reopen_files = false;
+
 	cifs_dbg(FYI, "Reopen persistent handles");
 	INIT_LIST_HEAD(&tmp_list);
 
@@ -793,7 +798,8 @@ cifs_reopen_persistent_handles(struct cifs_tcon *tcon)
 
 	list_for_each_safe(tmp, tmp1, &tmp_list) {
 		open_file = list_entry(tmp, struct cifsFileInfo, rlist);
-		cifs_reopen_file(open_file, false /* do not flush */);
+		if (cifs_reopen_file(open_file, false /* do not flush */))
+			tcon->need_reopen_files = true;
 		list_del_init(&open_file->rlist);
 		cifsFileInfo_put(open_file);
 	}
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 4ba3f68..8745722 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -250,16 +250,19 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon)
 	}
 
 	cifs_mark_open_files_invalid(tcon);
+	if (tcon->use_persistent)
+		tcon->need_reopen_files = true;
 
 	rc = SMB2_tcon(0, tcon->ses, tcon->treeName, tcon, nls_codepage);
 	mutex_unlock(&tcon->ses->session_mutex);
 
-	if (tcon->use_persistent)
-		cifs_reopen_persistent_handles(tcon);
-
 	cifs_dbg(FYI, "reconnect tcon rc = %d\n", rc);
 	if (rc)
 		goto out;
+
+	if (smb2_command != SMB2_INTERNAL_CMD)
+		queue_delayed_work(cifsiod_wq, &server->reconnect, 0);
+
 	atomic_inc(&tconInfoReconnectCount);
 out:
 	/*
@@ -1990,7 +1993,7 @@ void smb2_reconnect_server(struct work_struct *work)
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) {
 		list_for_each_entry(tcon, &ses->tcon_list, tcon_list) {
-			if (tcon->need_reconnect) {
+			if (tcon->need_reconnect || tcon->need_reopen_files) {
 				tcon->tc_count++;
 				list_add_tail(&tcon->rlist, &tmp_list);
 				tcon_exist = true;
@@ -2007,7 +2010,8 @@ void smb2_reconnect_server(struct work_struct *work)
 	spin_unlock(&cifs_tcp_ses_lock);
 
 	list_for_each_entry_safe(tcon, tcon2, &tmp_list, rlist) {
-		smb2_reconnect(SMB2_ECHO, tcon);
+		if (!smb2_reconnect(SMB2_INTERNAL_CMD, tcon))
+			cifs_reopen_persistent_handles(tcon);
 		list_del_init(&tcon->rlist);
 		cifs_put_tcon(tcon);
 	}
diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
index fd3709e..dc0d141 100644
--- a/fs/cifs/smb2pdu.h
+++ b/fs/cifs/smb2pdu.h
@@ -80,6 +80,8 @@
 #define SMB2_SET_INFO		cpu_to_le16(SMB2_SET_INFO_HE)
 #define SMB2_OPLOCK_BREAK	cpu_to_le16(SMB2_OPLOCK_BREAK_HE)
 
+#define SMB2_INTERNAL_CMD	cpu_to_le16(0xFFFF)
+
 #define NUMBER_OF_SMB2_COMMANDS	0x0013
 
 /* BB FIXME - analyze following length BB */
-- 
2.7.4

Re: [PATCH v2 3/5] CIFS: Fix a possible memory corruption in push locks

[Sachin Prabhu]

On Mon, 2016-12-05 at 13:11 -0800, Pavel Shilovsky wrote:
> If maxBuf is not 0 but less than a size of SMB2 lock structure
> we can end up with a memory corruption.
> 
> Cc: Stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Signed-off-by: Pavel Shilovsky <pshilov-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>


Acked-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

> ---
>  fs/cifs/smb2file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/cifs/smb2file.c b/fs/cifs/smb2file.c
> index f9e766f..b2aff0c 100644
> --- a/fs/cifs/smb2file.c
> +++ b/fs/cifs/smb2file.c
> @@ -260,7 +260,7 @@ smb2_push_mandatory_locks(struct cifsFileInfo
> *cfile)
>  	 * and check it for zero before using.
>  	 */
>  	max_buf = tlink_tcon(cfile->tlink)->ses->server->maxBuf;
> -	if (!max_buf) {
> +	if (max_buf < sizeof(struct smb2_lock_element)) {
>  		free_xid(xid);
>  		return -EINVAL;
>  	}

Re: [PATCH v2 4/5] CIFS: Fix a possible memory corruption during reconnect

[Sachin Prabhu]

On Mon, 2016-12-05 at 13:11 -0800, Pavel Shilovsky wrote:
> We can not unlock/lock cifs_tcp_ses_lock while walking through ses
> and tcon lists because it can corrupt list iterator pointers and
> a tcon structure can be released if we don't hold an extra reference.
> Fix it by moving a reconnect process to a separate delayed work
> and acquiring a reference to every tcon that needs to be reconnected.
> Also do not send an echo request on newly established connections.
> 
> CC: Stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Signed-off-by: Pavel Shilovsky <pshilov-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>
> ---
>  fs/cifs/cifsglob.h  |  3 +++
>  fs/cifs/cifsproto.h |  3 +++
>  fs/cifs/connect.c   | 34 +++++++++++++++++++-----
>  fs/cifs/smb2pdu.c   | 75 ++++++++++++++++++++++++++++++++++++-------
> ----------
>  fs/cifs/smb2proto.h |  1 +
>  5 files changed, 85 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 3e95191..89a0d7f 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -647,6 +647,8 @@ struct TCP_Server_Info {
>  	unsigned int	max_read;
>  	unsigned int	max_write;
>  	__u8		preauth_hash[512];
> +	struct delayed_work reconnect; /* reconnect workqueue job */
> +	struct mutex reconnect_mutex; /* prevent simultaneous
> reconnects */
>  #endif /* CONFIG_CIFS_SMB2 */
>  	unsigned long echo_interval;
>  };
> @@ -850,6 +852,7 @@ cap_unix(struct cifs_ses *ses)
>  struct cifs_tcon {
>  	struct list_head tcon_list;
>  	int tc_count;
> +	struct list_head rlist; /* reconnect list */
>  	struct list_head openFileList;
>  	spinlock_t open_file_lock; /* protects list above */
>  	struct cifs_ses *ses;	/* pointer to session
> associated with */
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index ced0e42..cd8025a 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -206,6 +206,9 @@ extern void cifs_add_pending_open_locked(struct
> cifs_fid *fid,
>  					 struct tcon_link *tlink,
>  					 struct cifs_pending_open
> *open);
>  extern void cifs_del_pending_open(struct cifs_pending_open *open);
> +extern void cifs_put_tcp_session(struct TCP_Server_Info *server,
> +				 int from_reconnect);
> +extern void cifs_put_tcon(struct cifs_tcon *tcon);
>  
>  #if IS_ENABLED(CONFIG_CIFS_DFS_UPCALL)
>  extern void cifs_dfs_release_automount_timer(void);
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 5563de3..5aaf7b1 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -52,6 +52,9 @@
>  #include "nterr.h"
>  #include "rfc1002pdu.h"
>  #include "fscache.h"
> +#ifdef CONFIG_CIFS_SMB2
> +#include "smb2proto.h"
> +#endif
>  
>  #define CIFS_PORT 445
>  #define RFC1001_PORT 139
> @@ -2110,8 +2113,8 @@ cifs_find_tcp_session(struct smb_vol *vol)
>  	return NULL;
>  }
>  
> -static void
> -cifs_put_tcp_session(struct TCP_Server_Info *server)
> +void
> +cifs_put_tcp_session(struct TCP_Server_Info *server, int
> from_reconnect)

The from_reconnect here becomes an unused variable if the kernel is
built without CONFIG_CIFS_SMB2 support. 

>  {
>  	struct task_struct *task;
>  
> @@ -2128,6 +2131,19 @@ cifs_put_tcp_session(struct TCP_Server_Info
> *server)
>  
>  	cancel_delayed_work_sync(&server->echo);
>  
> +#ifdef CONFIG_CIFS_SMB2
> +	if (from_reconnect)
> +		/*
> +		 * Avoid deadlock here: reconnect work calls
> +		 * cifs_put_tcp_session() at its end. Need to be
> sure
> +		 * that reconnect work does nothing with server
> pointer after
> +		 * that step.
> +		 */
> +		cancel_delayed_work(&server->reconnect);
> +	else
> +		cancel_delayed_work_sync(&server->reconnect);
> +#endif
> +
>  	spin_lock(&GlobalMid_Lock);
>  	server->tcpStatus = CifsExiting;
>  	spin_unlock(&GlobalMid_Lock);
> @@ -2192,6 +2208,10 @@ cifs_get_tcp_session(struct smb_vol
> *volume_info)
>  	INIT_LIST_HEAD(&tcp_ses->tcp_ses_list);
>  	INIT_LIST_HEAD(&tcp_ses->smb_ses_list);
>  	INIT_DELAYED_WORK(&tcp_ses->echo, cifs_echo_request);
> +#ifdef CONFIG_CIFS_SMB2
> +	INIT_DELAYED_WORK(&tcp_ses->reconnect,
> smb2_reconnect_server);
> +	mutex_init(&tcp_ses->reconnect_mutex);
> +#endif
>  	memcpy(&tcp_ses->srcaddr, &volume_info->srcaddr,
>  	       sizeof(tcp_ses->srcaddr));
>  	memcpy(&tcp_ses->dstaddr, &volume_info->dstaddr,
> @@ -2350,7 +2370,7 @@ cifs_put_smb_ses(struct cifs_ses *ses)
>  	spin_unlock(&cifs_tcp_ses_lock);
>  
>  	sesInfoFree(ses);
> -	cifs_put_tcp_session(server);
> +	cifs_put_tcp_session(server, 0);
>  }
>  
>  #ifdef CONFIG_KEYS
> @@ -2524,7 +2544,7 @@ cifs_get_smb_ses(struct TCP_Server_Info
> *server, struct smb_vol *volume_info)
>  		mutex_unlock(&ses->session_mutex);
>  
>  		/* existing SMB ses has a server reference already
> */
> -		cifs_put_tcp_session(server);
> +		cifs_put_tcp_session(server, 0);
>  		free_xid(xid);
>  		return ses;
>  	}
> @@ -2620,7 +2640,7 @@ cifs_find_tcon(struct cifs_ses *ses, struct
> smb_vol *volume_info)
>  	return NULL;
>  }
>  
> -static void
> +void
>  cifs_put_tcon(struct cifs_tcon *tcon)
>  {
>  	unsigned int xid;
> @@ -3824,7 +3844,7 @@ cifs_mount(struct cifs_sb_info *cifs_sb, struct
> smb_vol *volume_info)
>  		else if (ses)
>  			cifs_put_smb_ses(ses);
>  		else
> -			cifs_put_tcp_session(server);
> +			cifs_put_tcp_session(server, 0);
>  		bdi_destroy(&cifs_sb->bdi);
>  	}
>  
> @@ -4135,7 +4155,7 @@ cifs_construct_tcon(struct cifs_sb_info
> *cifs_sb, kuid_t fsuid)
>  	ses = cifs_get_smb_ses(master_tcon->ses->server, vol_info);
>  	if (IS_ERR(ses)) {
>  		tcon = (struct cifs_tcon *)ses;
> -		cifs_put_tcp_session(master_tcon->ses->server);
> +		cifs_put_tcp_session(master_tcon->ses->server, 0);
>  		goto out;
>  	}
>  
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 730d57b..4ba3f68 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -1972,6 +1972,54 @@ smb2_echo_callback(struct mid_q_entry *mid)
>  	add_credits(server, credits_received, CIFS_ECHO_OP);
>  }
>  
> +void smb2_reconnect_server(struct work_struct *work)
> +{
> +	struct TCP_Server_Info *server = container_of(work,
> +					struct TCP_Server_Info,
> reconnect.work);
> +	struct cifs_ses *ses;
> +	struct cifs_tcon *tcon, *tcon2;
> +	struct list_head tmp_list;
> +	int tcon_exist = false;
> +
> +	/* Prevent simultaneous reconnects that can corrupt tcon-
> >rlist list */
> +	mutex_lock(&server->reconnect_mutex);
> +
> +	INIT_LIST_HEAD(&tmp_list);
> +	cifs_dbg(FYI, "Need negotiate, reconnecting tcons\n");
> +
> +	spin_lock(&cifs_tcp_ses_lock);
> +	list_for_each_entry(ses, &server->smb_ses_list,
> smb_ses_list) {
> +		list_for_each_entry(tcon, &ses->tcon_list,
> tcon_list) {
> +			if (tcon->need_reconnect) {
> +				tcon->tc_count++;
> +				list_add_tail(&tcon->rlist,
> &tmp_list);
> +				tcon_exist = true;
> +			}
> +		}
> +	}
> +	/*
> +	 * Get the reference to server struct to be sure that the
> last call of
> +	 * cifs_put_tcon() in the loop below won't release the
> server pointer.
> +	 */
> +	if (tcon_exist)
> +		server->srv_count++;
> +
> +	spin_unlock(&cifs_tcp_ses_lock);
> +
> +	list_for_each_entry_safe(tcon, tcon2, &tmp_list, rlist) {
> +		smb2_reconnect(SMB2_ECHO, tcon);
> +		list_del_init(&tcon->rlist);
> +		cifs_put_tcon(tcon);
> +	}
> +
> +	cifs_dbg(FYI, "Reconnecting tcons finished\n");
> +	mutex_unlock(&server->reconnect_mutex);
> +
> +	/* now we can safely release srv struct */
> +	if (tcon_exist)
> +		cifs_put_tcp_session(server, 1);
> +}
> +
>  int
>  SMB2_echo(struct TCP_Server_Info *server)
>  {
> @@ -1984,32 +2032,11 @@ SMB2_echo(struct TCP_Server_Info *server)
>  	cifs_dbg(FYI, "In echo request\n");
>  
>  	if (server->tcpStatus == CifsNeedNegotiate) {
> -		struct list_head *tmp, *tmp2;
> -		struct cifs_ses *ses;
> -		struct cifs_tcon *tcon;
> -
> -		cifs_dbg(FYI, "Need negotiate, reconnecting
> tcons\n");
> -		spin_lock(&cifs_tcp_ses_lock);
> -		list_for_each(tmp, &server->smb_ses_list) {
> -			ses = list_entry(tmp, struct cifs_ses,
> smb_ses_list);
> -			list_for_each(tmp2, &ses->tcon_list) {
> -				tcon = list_entry(tmp2, struct
> cifs_tcon,
> -						  tcon_list);
> -				/* add check for persistent handle
> reconnect */
> -				if (tcon && tcon->need_reconnect) {
> -					spin_unlock(&cifs_tcp_ses_lo
> ck);
> -					rc =
> smb2_reconnect(SMB2_ECHO, tcon);
> -					spin_lock(&cifs_tcp_ses_lock
> );
> -				}
> -			}
> -		}
> -		spin_unlock(&cifs_tcp_ses_lock);
> +		/* No need to send echo on newly established
> connections */
> +		queue_delayed_work(cifsiod_wq, &server->reconnect,
> 0);
> +		return rc;
>  	}
>  
> -	/* if no session, renegotiate failed above */
> -	if (server->tcpStatus == CifsNeedNegotiate)
> -		return -EIO;
> -
>  	rc = small_smb2_init(SMB2_ECHO, NULL, (void **)&req);
>  	if (rc)
>  		return rc;
> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> index eb2cde2..f2d511a 100644
> --- a/fs/cifs/smb2proto.h
> +++ b/fs/cifs/smb2proto.h
> @@ -96,6 +96,7 @@ extern int smb2_open_file(const unsigned int xid,
>  extern int smb2_unlock_range(struct cifsFileInfo *cfile,
>  			     struct file_lock *flock, const unsigned
> int xid);
>  extern int smb2_push_mandatory_locks(struct cifsFileInfo *cfile);
> +extern void smb2_reconnect_server(struct work_struct *work);
>  
>  /*
>   * SMB2 Worker functions - most of protocol specific implementation
> details

Apart from that small nitpick,

Acked-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>