| Message ID | CAH2r5muOpGHkvPyxJy_Qbp7gLC4ssL4eBOcY59U9+7KAjFHzcQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [CIFS] make locking consistent around the server session status | expand |
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,
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) >
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
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
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(-) }