[PATCH] cifs: fix potential double put of TCP session reference

[PATCH] cifs: fix potential double put of TCP session reference

[Jeff Layton]

cifs_get_smb_ses must be called on a server pointer on which it holds an
active reference. It first does a search for an existing SMB session. If
it finds one, it'll put the server reference and then try to ensure that
the negprot is done, etc.

If it encounters an error at that point then it'll return an error.
There's a potential problem here though. When cifs_get_smb_ses returns
an error, the caller will also put the TCP server reference leading to a
double-put.

Fix this by having cifs_get_smb_ses only put the server reference if
it found an existing session that it could use and isn't returning an
error.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 67dad54..88c84a3 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1706,9 +1706,6 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
 	if (ses) {
 		cFYI(1, "Existing smb sess found (status=%d)", ses->status);
 
-		/* existing SMB ses has a server reference already */
-		cifs_put_tcp_session(server);
-
 		mutex_lock(&ses->session_mutex);
 		rc = cifs_negotiate_protocol(xid, ses);
 		if (rc) {
@@ -1731,6 +1728,9 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
 			}
 		}
 		mutex_unlock(&ses->session_mutex);
+
+		/* existing SMB ses has a server reference already */
+		cifs_put_tcp_session(server);
 		FreeXid(xid);
 		return ses;
 	}
-- 
1.7.2.2

Re: [PATCH] cifs: fix potential double put of TCP session reference

[Suresh Jayaraman]

On 09/13/2010 11:32 PM, Jeff Layton wrote:
> cifs_get_smb_ses must be called on a server pointer on which it holds an
> active reference. It first does a search for an existing SMB session. If
> it finds one, it'll put the server reference and then try to ensure that
> the negprot is done, etc.
> 
> If it encounters an error at that point then it'll return an error.
> There's a potential problem here though. When cifs_get_smb_ses returns
> an error, the caller will also put the TCP server reference leading to a
> double-put.
> 
> Fix this by having cifs_get_smb_ses only put the server reference if
> it found an existing session that it could use and isn't returning an
> error.
> 
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/connect.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 67dad54..88c84a3 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -1706,9 +1706,6 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
>  	if (ses) {
>  		cFYI(1, "Existing smb sess found (status=%d)", ses->status);
>  
> -		/* existing SMB ses has a server reference already */
> -		cifs_put_tcp_session(server);
> -
>  		mutex_lock(&ses->session_mutex);
>  		rc = cifs_negotiate_protocol(xid, ses);
>  		if (rc) {
> @@ -1731,6 +1728,9 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
>  			}
>  		}
>  		mutex_unlock(&ses->session_mutex);
> +
> +		/* existing SMB ses has a server reference already */
> +		cifs_put_tcp_session(server);
>  		FreeXid(xid);
>  		return ses;
>  	}

Looks correct to me.

Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

Re: [PATCH] cifs: fix potential double put of TCP session reference

[Jeff Layton]

On Tue, 14 Sep 2010 15:18:32 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:

> On 09/13/2010 11:32 PM, Jeff Layton wrote:
> > cifs_get_smb_ses must be called on a server pointer on which it holds an
> > active reference. It first does a search for an existing SMB session. If
> > it finds one, it'll put the server reference and then try to ensure that
> > the negprot is done, etc.
> > 
> > If it encounters an error at that point then it'll return an error.
> > There's a potential problem here though. When cifs_get_smb_ses returns
> > an error, the caller will also put the TCP server reference leading to a
> > double-put.
> > 
> > Fix this by having cifs_get_smb_ses only put the server reference if
> > it found an existing session that it could use and isn't returning an
> > error.
> > 
> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  fs/cifs/connect.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index 67dad54..88c84a3 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -1706,9 +1706,6 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
> >  	if (ses) {
> >  		cFYI(1, "Existing smb sess found (status=%d)", ses->status);
> >  
> > -		/* existing SMB ses has a server reference already */
> > -		cifs_put_tcp_session(server);
> > -
> >  		mutex_lock(&ses->session_mutex);
> >  		rc = cifs_negotiate_protocol(xid, ses);
> >  		if (rc) {
> > @@ -1731,6 +1728,9 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
> >  			}
> >  		}
> >  		mutex_unlock(&ses->session_mutex);
> > +
> > +		/* existing SMB ses has a server reference already */
> > +		cifs_put_tcp_session(server);
> >  		FreeXid(xid);
> >  		return ses;
> >  	}
> 
> Looks correct to me.
> 
> Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

Thanks. In hindsight, I should have probably sent this to stable too.

Steve, would you like me to resend and cc stable? We probably want this
in 2.6.36 too, if possible.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifs: fix potential double put of TCP session reference

[Steve French]

On Tue, Sep 14, 2010 at 6:39 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue, 14 Sep 2010 15:18:32 +0530
> Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
>
>> On 09/13/2010 11:32 PM, Jeff Layton wrote:
>> > cifs_get_smb_ses must be called on a server pointer on which it holds an
>> > active reference. It first does a search for an existing SMB session. If
>> > it finds one, it'll put the server reference and then try to ensure that
>> > the negprot is done, etc.
>> >
>> > If it encounters an error at that point then it'll return an error.
>> > There's a potential problem here though. When cifs_get_smb_ses returns
>> > an error, the caller will also put the TCP server reference leading to a
>> > double-put.
>> >
>> > Fix this by having cifs_get_smb_ses only put the server reference if
>> > it found an existing session that it could use and isn't returning an
>> > error.
>> >
>> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> > ---
>> >  fs/cifs/connect.c |    6 +++---
>> >  1 files changed, 3 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> > index 67dad54..88c84a3 100644
>> > --- a/fs/cifs/connect.c
>> > +++ b/fs/cifs/connect.c
>> > @@ -1706,9 +1706,6 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
>> >     if (ses) {
>> >             cFYI(1, "Existing smb sess found (status=%d)", ses->status);
>> >
>> > -           /* existing SMB ses has a server reference already */
>> > -           cifs_put_tcp_session(server);
>> > -
>> >             mutex_lock(&ses->session_mutex);
>> >             rc = cifs_negotiate_protocol(xid, ses);
>> >             if (rc) {
>> > @@ -1731,6 +1728,9 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
>> >                     }
>> >             }
>> >             mutex_unlock(&ses->session_mutex);
>> > +
>> > +           /* existing SMB ses has a server reference already */
>> > +           cifs_put_tcp_session(server);
>> >             FreeXid(xid);
>> >             return ses;
>> >     }
>>
>> Looks correct to me.
>>
>> Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
>
> Thanks. In hindsight, I should have probably sent this to stable too.
>
> Steve, would you like me to resend and cc stable? We probably want this
> in 2.6.36 too, if possible.

Yes - I agree makes sense.  Any other candidate patches for 2.6.36?

-- 
Thanks,

Steve

Re: [PATCH] cifs: fix potential double put of TCP session reference

[Jeff Layton]

On Tue, 14 Sep 2010 10:19:56 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Tue, Sep 14, 2010 at 6:39 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Tue, 14 Sep 2010 15:18:32 +0530
> > Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
> >
> >> On 09/13/2010 11:32 PM, Jeff Layton wrote:
> >> > cifs_get_smb_ses must be called on a server pointer on which it holds an
> >> > active reference. It first does a search for an existing SMB session. If
> >> > it finds one, it'll put the server reference and then try to ensure that
> >> > the negprot is done, etc.
> >> >
> >> > If it encounters an error at that point then it'll return an error.
> >> > There's a potential problem here though. When cifs_get_smb_ses returns
> >> > an error, the caller will also put the TCP server reference leading to a
> >> > double-put.
> >> >
> >> > Fix this by having cifs_get_smb_ses only put the server reference if
> >> > it found an existing session that it could use and isn't returning an
> >> > error.
> >> >
> >> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> > ---
> >> >  fs/cifs/connect.c |    6 +++---
> >> >  1 files changed, 3 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> >> > index 67dad54..88c84a3 100644
> >> > --- a/fs/cifs/connect.c
> >> > +++ b/fs/cifs/connect.c
> >> > @@ -1706,9 +1706,6 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
> >> >     if (ses) {
> >> >             cFYI(1, "Existing smb sess found (status=%d)", ses->status);
> >> >
> >> > -           /* existing SMB ses has a server reference already */
> >> > -           cifs_put_tcp_session(server);
> >> > -
> >> >             mutex_lock(&ses->session_mutex);
> >> >             rc = cifs_negotiate_protocol(xid, ses);
> >> >             if (rc) {
> >> > @@ -1731,6 +1728,9 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
> >> >                     }
> >> >             }
> >> >             mutex_unlock(&ses->session_mutex);
> >> > +
> >> > +           /* existing SMB ses has a server reference already */
> >> > +           cifs_put_tcp_session(server);
> >> >             FreeXid(xid);
> >> >             return ses;
> >> >     }
> >>
> >> Looks correct to me.
> >>
> >> Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
> >
> > Thanks. In hindsight, I should have probably sent this to stable too.
> >
> > Steve, would you like me to resend and cc stable? We probably want this
> > in 2.6.36 too, if possible.
> 
> Yes - I agree makes sense.  Any other candidate patches for 2.6.36?
> 

No, that's all I've got for now. I'll resend in a bit and cc stable.

Thanks,
-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

[PATCH] cifs: fix potential double put of TCP session reference

[Jeff Layton]

cifs_get_smb_ses must be called on a server pointer on which it holds an
active reference. It first does a search for an existing SMB session. If
it finds one, it'll put the server reference and then try to ensure that
the negprot is done, etc.

If it encounters an error at that point then it'll return an error.
There's a potential problem here though. When cifs_get_smb_ses returns
an error, the caller will also put the TCP server reference leading to a
double-put.

Fix this by having cifs_get_smb_ses only put the server reference if
it found an existing session that it could use and isn't returning an
error.

Cc: stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 67dad54..88c84a3 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1706,9 +1706,6 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
 	if (ses) {
 		cFYI(1, "Existing smb sess found (status=%d)", ses->status);
 
-		/* existing SMB ses has a server reference already */
-		cifs_put_tcp_session(server);
-
 		mutex_lock(&ses->session_mutex);
 		rc = cifs_negotiate_protocol(xid, ses);
 		if (rc) {
@@ -1731,6 +1728,9 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
 			}
 		}
 		mutex_unlock(&ses->session_mutex);
+
+		/* existing SMB ses has a server reference already */
+		cifs_put_tcp_session(server);
 		FreeXid(xid);
 		return ses;
 	}
-- 
1.7.2.2