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

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

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>