There were three places where we were not taking the spinlock
around updates to server->tcpStatus when it was being modified.
To be consistent (also removes Coverity warning) and to remove
possibility of race best to lock all places where it is updated.
Addresses-Coverity: 1399512 ("Data race condition")
Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx>
---
fs/cifs/cifsglob.h | 3 ++-
fs/cifs/connect.c | 4 ++++
fs/cifs/transport.c | 2 ++
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 3100f8b66e60..921680fb7931 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -577,6 +577,7 @@ struct TCP_Server_Info {
char server_RFC1001_name[RFC1001_NAME_LEN_WITH_NULL];
struct smb_version_operations *ops;
struct smb_version_values *vals;
+ /* updates to tcpStatus protected by GlobalMid_Lock */
enum statusEnum tcpStatus; /* what we think the status is */
char *hostname; /* hostname portion of UNC string */
struct socket *ssocket;
@@ -1785,7 +1786,7 @@ require use of the stronger protocol */
* list operations on pending_mid_q and oplockQ
* updates to XID counters, multiplex id and SMB sequence numbers
* list operations on global DnotifyReqList
- * updates to ses->status
+ * updates to ses->status and TCP_Server_Info->tcpStatus
* updates to server->CurrentMid
* tcp_ses_lock protects:
* list operations on tcp and SMB session lists
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 5d269f583dac..944fb92f50c7 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1358,7 +1358,9 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx)
* to the struct since the kernel thread not created yet
* no need to spinlock this init of tcpStatus or srv_count
*/
+ spin_lock(&GlobalMid_Lock);
tcp_ses->tcpStatus = CifsNew;
+ spin_unlock(&GlobalMid_Lock);
++tcp_ses->srv_count;
if (ctx->echo_interval >= SMB_ECHO_INTERVAL_MIN &&
@@ -1403,7 +1405,9 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx)
goto out_err_crypto_release;
}
tcp_ses->min_offload = ctx->min_offload;
+ spin_lock(&GlobalMid_Lock);
tcp_ses->tcpStatus = CifsNeedNegotiate;
+ spin_unlock(&GlobalMid_Lock);
if ((ctx->max_credits < 20) || (ctx->max_credits > 60000))
tcp_ses->max_credits = SMB2_MAX_CREDITS_AVAILABLE;
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index f65f9a692ca2..75a95de320cf 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -431,7 +431,9 @@ __smb_send_rqst(struct TCP_Server_Info *server,
int num_rqst,
* be taken as the remainder of this one. We need to kill the
* socket so the server throws away the partial SMB
*/
+ spin_lock(&GlobalMid_Lock);
server->tcpStatus = CifsNeedReconnect;
+ spin_unlock(&GlobalMid_Lock);
trace_smb3_partial_send_reconnect(server->CurrentMid,
server->conn_id, server->hostname);
}
--
Thanks,
Steve
From dc43ee1df9b95c4d9e17285926d64c4600340f1e Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@xxxxxxxxxxxxx>
Date: Thu, 1 Jul 2021 12:22:47 -0500
Subject: [PATCH] cifs: make locking consistent around the server session
status
There were three places where we were not taking the spinlock
around updates to server->tcpStatus when it was being modified.
To be consistent (also removes Coverity warning) and to remove
possibility of race best to lock all places where it is updated.
Addresses-Coverity: 1399512 ("Data race condition")
Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx>
---
fs/cifs/cifsglob.h | 3 ++-
fs/cifs/connect.c | 4 ++++
fs/cifs/transport.c | 2 ++
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 3100f8b66e60..921680fb7931 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -577,6 +577,7 @@ struct TCP_Server_Info {
char server_RFC1001_name[RFC1001_NAME_LEN_WITH_NULL];
struct smb_version_operations *ops;
struct smb_version_values *vals;
+ /* updates to tcpStatus protected by GlobalMid_Lock */
enum statusEnum tcpStatus; /* what we think the status is */
char *hostname; /* hostname portion of UNC string */
struct socket *ssocket;
@@ -1785,7 +1786,7 @@ require use of the stronger protocol */
* list operations on pending_mid_q and oplockQ
* updates to XID counters, multiplex id and SMB sequence numbers
* list operations on global DnotifyReqList
- * updates to ses->status
+ * updates to ses->status and TCP_Server_Info->tcpStatus
* updates to server->CurrentMid
* tcp_ses_lock protects:
* list operations on tcp and SMB session lists
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 5d269f583dac..944fb92f50c7 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1358,7 +1358,9 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx)
* to the struct since the kernel thread not created yet
* no need to spinlock this init of tcpStatus or srv_count
*/
+ spin_lock(&GlobalMid_Lock);
tcp_ses->tcpStatus = CifsNew;
+ spin_unlock(&GlobalMid_Lock);
++tcp_ses->srv_count;
if (ctx->echo_interval >= SMB_ECHO_INTERVAL_MIN &&
@@ -1403,7 +1405,9 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx)
goto out_err_crypto_release;
}
tcp_ses->min_offload = ctx->min_offload;
+ spin_lock(&GlobalMid_Lock);
tcp_ses->tcpStatus = CifsNeedNegotiate;
+ spin_unlock(&GlobalMid_Lock);
if ((ctx->max_credits < 20) || (ctx->max_credits > 60000))
tcp_ses->max_credits = SMB2_MAX_CREDITS_AVAILABLE;
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index f65f9a692ca2..75a95de320cf 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -431,7 +431,9 @@ __smb_send_rqst(struct TCP_Server_Info *server, int num_rqst,
* be taken as the remainder of this one. We need to kill the
* socket so the server throws away the partial SMB
*/
+ spin_lock(&GlobalMid_Lock);
server->tcpStatus = CifsNeedReconnect;
+ spin_unlock(&GlobalMid_Lock);
trace_smb3_partial_send_reconnect(server->CurrentMid,
server->conn_id, server->hostname);
}
--
2.30.2
