[PATCH][CIFS] make locking consistent around the server session status

[PATCH][CIFS] make locking consistent around the server session status

[Steve French]

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

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@microsoft.com>
---
 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

[-- Attachment #2: 0001-cifs-make-locking-consistent-around-the-server-sessi.patch --]
[-- Type: text/x-patch, Size: 3160 bytes --]

From dc43ee1df9b95c4d9e17285926d64c4600340f1e Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
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@microsoft.com>
---
 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

Re: [PATCH][CIFS] make locking consistent around the server session status

[Aurélien Aptel]

Steve French <smfrench@gmail.com> writes:
> 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.

If we lock for writing we also need to lock for reading otherwise the
locking isn't protecting anything.
> --- 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

    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

There is comment just on top saying no need to spinlock.

>   */
> + spin_lock(&GlobalMid_Lock);
>   tcp_ses->tcpStatus = CifsNew;
> + spin_unlock(&GlobalMid_Lock);
>   ++tcp_ses->srv_count;

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Re: [PATCH][CIFS] make locking consistent around the server session status

[Steve French]

I removed the chunk you noted - whether it is worth updating the other
50+ where we check, not sure it is worth doing that if Coverity
doesn't complain and it isn't an issue in practice why change the
other 50 (if on the other hand there is much risk then it would be
worth making the bigger change)?

Many of the checks are like:

if (tcon->ses->server->tcpStatus != CifsExiting)
or
if (server->tcpStatus == CifsNeedReconnect)
or
if ((server->tcpStatus == CifsGood)...

so they are unlikely to be a problem if we end up with an unknown
value briefly while a read overlaps an update to tcpStatus

If you are in favor of the bigger change I am ok with it - but my goal
in this was more to "remove distracting coverity messages" so we can
spot the dangerous ones more quickly (it has spotted a few serious
problems this year so it is worth checking - but the old warnings were
distracting)


> @@ -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;

On Fri, Jul 2, 2021 at 6:04 AM Aurélien Aptel <aaptel@suse.com> wrote:
>
> Steve French <smfrench@gmail.com> writes:
> > 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.
>
> If we lock for writing we also need to lock for reading otherwise the
> locking isn't protecting anything.
> > --- 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
>
>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> There is comment just on top saying no need to spinlock.
>
> >   */
> > + spin_lock(&GlobalMid_Lock);
> >   tcp_ses->tcpStatus = CifsNew;
> > + spin_unlock(&GlobalMid_Lock);
> >   ++tcp_ses->srv_count;
>
> Cheers,
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
>


-- 
Thanks,

Steve

Re: [PATCH][CIFS] make locking consistent around the server session status

[Steve French]

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

lightly updated with Paulo's suggestion.

On Fri, Jul 2, 2021 at 11:39 AM Steve French <smfrench@gmail.com> wrote:
>
> I removed the chunk you noted - whether it is worth updating the other
> 50+ where we check, not sure it is worth doing that if Coverity
> doesn't complain and it isn't an issue in practice why change the
> other 50 (if on the other hand there is much risk then it would be
> worth making the bigger change)?
>
> Many of the checks are like:
>
> if (tcon->ses->server->tcpStatus != CifsExiting)
> or
> if (server->tcpStatus == CifsNeedReconnect)
> or
> if ((server->tcpStatus == CifsGood)...
>
> so they are unlikely to be a problem if we end up with an unknown
> value briefly while a read overlaps an update to tcpStatus
>
> If you are in favor of the bigger change I am ok with it - but my goal
> in this was more to "remove distracting coverity messages" so we can
> spot the dangerous ones more quickly (it has spotted a few serious
> problems this year so it is worth checking - but the old warnings were
> distracting)
>
>
> > @@ -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;
>
> On Fri, Jul 2, 2021 at 6:04 AM Aurélien Aptel <aaptel@suse.com> wrote:
> >
> > Steve French <smfrench@gmail.com> writes:
> > > 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.
> >
> > If we lock for writing we also need to lock for reading otherwise the
> > locking isn't protecting anything.
> > > --- 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
> >
> >     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > There is comment just on top saying no need to spinlock.
> >
> > >   */
> > > + spin_lock(&GlobalMid_Lock);
> > >   tcp_ses->tcpStatus = CifsNew;
> > > + spin_unlock(&GlobalMid_Lock);
> > >   ++tcp_ses->srv_count;
> >
> > Cheers,
> > --
> > Aurélien Aptel / SUSE Labs Samba Team
> > GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
> >
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

[-- Attachment #2: 0001-cifs-make-locking-consistent-around-the-server-sessi.patch --]
[-- Type: text/x-patch, Size: 3003 bytes --]

From 01cf30825c8729884090151ab97f1c9c5d14a8bc Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
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.
Two of the three were in initialization of the field and can't
race - but added lock around the other.

Addresses-Coverity: 1399512 ("Data race condition")
Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/cifsglob.h  | 3 ++-
 fs/cifs/connect.c   | 5 +++++
 fs/cifs/transport.c | 2 ++
 3 files changed, 9 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..01dc45178f66 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1403,6 +1403,11 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx)
 		goto out_err_crypto_release;
 	}
 	tcp_ses->min_offload = ctx->min_offload;
+	/*
+	 * at this point we are the only ones with the pointer
+	 * to the struct since the kernel thread not created yet
+	 * no need to spinlock this update of tcpStatus
+	 */
 	tcp_ses->tcpStatus = CifsNeedNegotiate;
 
 	if ((ctx->max_credits < 20) || (ctx->max_credits > 60000))
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