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@xxxxxxxx> wrote: > > Steve French <smfrench@xxxxxxxxx> 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
