On Wed, Oct 13, 2010 at 1:47 PM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> On Wed, 13 Oct 2010 13:42:24 -0500
> Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
>
>> On Wed, Oct 13, 2010 at 12:03 PM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
>> > On Wed, 13 Oct 2010 10:28:25 -0500
>> > Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
>> >
>> >> On Wed, Oct 13, 2010 at 7:36 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
>> >> > On Tue, 12 Oct 2010 14:45:03 -0500
>> >> > shirishpargaonkar@xxxxxxxxx wrote:
>> >> >
>> >> >> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>> >> >>
>> >> >> 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 <shirishpargaonkar@xxxxxxxxx>
>> >> >
>> >> >
>> >> >> 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 <jlayton@xxxxxxxxx>
>> >
>>
>> Or we could move them earlier
>>
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index 848e458..d7e0651 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -173,6 +173,8 @@ cifs_reconnect(struct TCP_Server_Info *server)
>> sock_release(server->ssocket);
>> server->ssocket = NULL;
>> }
>> + server->sequence_number = 0;
>> + server->session_estab = false;
>>
>> spin_lock(&GlobalMid_Lock);
>> list_for_each(tmp, &server->pending_mid_q) {
>> @@ -205,8 +207,6 @@ cifs_reconnect(struct TCP_Server_Info *server)
>> spin_lock(&GlobalMid_Lock);
>> 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);
>>
>>
>> There is no smb connection at this point and (certainly) sessions are
>> not established at this point.
>>
>
> I think that's the right idea.
>
> You want to make sure that everything is properly torn down, flags
> reset, etc. before the socket is reconnected.
>
> There may be other pieces of cifs_reconnect that violate that rule as
> well. If there are, I'd suggest fixing those in a separate patch that
> goes before this one.
>
> --
> Jeff Layton <jlayton@xxxxxxxxx>
>
I think we can prevent race in cifs_reconnect with this patch.
Why allow more than one reconnect call to execute rest of the code at all?
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 848e458..3656c80 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -136,7 +136,8 @@ cifs_reconnect(struct TCP_Server_Info *server)
struct mid_q_entry *mid_entry;
spin_lock(&GlobalMid_Lock);
- if (server->tcpStatus == CifsExiting) {
+ if (server->tcpStatus == CifsExiting ||
+ server->tcpStatus == CifsNeedReconnect) {
/* the demux thread will exit normally
next time through the loop */
spin_unlock(&GlobalMid_Lock);
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
