On Wed, Oct 13, 2010 at 12:03 PM, Jeff Layton <[email protected]> wrote:
> On Wed, 13 Oct 2010 10:28:25 -0500
> Shirish Pargaonkar <[email protected]> wrote:
>
>> On Wed, Oct 13, 2010 at 7:36 AM, Jeff Layton <[email protected]> wrote:
>> > On Tue, 12 Oct 2010 14:45:03 -0500
>> > [email protected] wrote:
>> >
>> >> From: Shirish Pargaonkar <[email protected]>
>> >>
>> >> Start calculation auth response within a session. Move/Add pertinet
>> >> data structures like session key, server challenge and ntlmv2_hash in
>> >> a session structure. We should do the calculations within a session
>> >> before copying session key and response over to server data
>> >> structures because a session setup can fail.
>> >>
>> >> Only after a very first smb session succeeds, it copies/makes its
>> >> session key, session key of smb connection. This key stays with
>> >> the smb connection throughout its life.
>> >>
>> >>
>> >> Signed-off-by: Shirish Pargaonkar <[email protected]>
>> >
>> >
>> >> bcc_ptr += CIFS_SESS_KEY_SIZE;
>> >> if (ses->capabilities & CAP_UNICODE) {
>> >> @@ -3175,6 +3176,15 @@ int cifs_setup_session(unsigned int xid, struct
>> >> cifsSesInfo *ses,
>> >> if (rc) {
>> >> cERROR(1, "Send error in SessSetup = %d", rc);
>> >> } else {
>> >> + mutex_lock(&ses->server->srv_mutex);
>> >> + if (!server->session_estab) {
>> >> + memcpy(&server->session_key.data,
>> >> + &ses->auth_key.data, ses->auth_key.len);
>> >> + server->session_key.len = ses->auth_key.len;
>> >> + ses->server->session_estab = true;
>> > ^^^^
>> > nit: Aren't "server" and "ses->server" the
>> > same? Why access them differently here?
>> >
>> >> + }
>> >> + mutex_unlock(&server->srv_mutex);
>> >> +
>> > ^^^^^^
>> > What are the rules for locking around the session_estab
>> > flag? Here you're holding the srv_mutex when checking
>> > and altering it, but in cifs_reconnect you're altering
>> > it without holding the mutex. Is that a potential race
>> > condition?
>>
>> Jeff, I do not think it is a potential race. In cifs_reconnect, by the
>> time session_estab is set as false, all the sessions have been
>> invalidated and socket connection has been taken down
>> and restablished. Isn't that as good as new connection?
>> Setting session_estab under srv_mutex is mainly to prevent
>> newly established sessions from racing.
>>
>
> You may be right, but I don't immediately see what prevents the
> following scenario:
>
> The sessions have been invalidated in cifs_reconnect and we're making
> our way down to the point where that flag is set to false.
>
> Another task then calls smb_init to issue a new call, notices that the
> session is invalid and reconnects it.
>
> This succeeds, but cifs_reconnect isn't done yet. session_estab is
> still true. The session setup code takes the mutex and then checks and
> finds that the flag is still true so it doesn't copy the key.
>
> Eventually cifs_reconnect flips the flag to false (maybe even while
> we're holding the mutex) but now we're borked. We have a valid session
> on the socket but the key is likely to be wrong.
>
> This is admittedly unlikely, and maybe impossible, but we should
> understand what makes it impossible before we wave our hands and say
> there's no race.
>
> --
> Jeff Layton <[email protected]>
>
I think this change should take care of that
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 848e458..613741b 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -203,10 +203,12 @@ cifs_reconnect(struct TCP_Server_Info *server)
} else {
atomic_inc(&tcpSesReconnectCount);
spin_lock(&GlobalMid_Lock);
+ if (server->tcpStatus == CifsNeedReconnect) {
+ server->sequence_number = 0;
+ server->session_estab = false;
+ }
if (server->tcpStatus != CifsExiting)
server->tcpStatus = CifsGood;
- server->sequence_number = 0;
- server->session_estab = false;
spin_unlock(&GlobalMid_Lock);
/* atomic_set(&server->inFlight,0);*/
wake_up(&server->response_q);
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
