diff mbox series

[v1] CIFS: prevent refcount underflow

Message ID 20190326123921.4116-1-aaptel@suse.com (mailing list archive)
State New, archived
Headers show
Series [v1] CIFS: prevent refcount underflow | expand

Commit Message

Aurélien Aptel March 26, 2019, 12:39 p.m. UTC
Replace kref_t by a simple refcount. We do not care about the
atomicity of the operation as long as the mutex is held.

This fixes a false-positive memory leak warning from kref_put() where
we close the cached fid twice and make the kref underflow.

Link: https://lore.kernel.org/linux-cifs/20190319115151.GA2092@light.dominikbrodowski.net/

Signed-off-by: Aurelien Aptel <aaptel@suse.com>
---
 fs/cifs/cifsglob.h |  2 +-
 fs/cifs/smb2ops.c  | 34 ++++++++++++++--------------------
 2 files changed, 15 insertions(+), 21 deletions(-)

Comments

Dominik Brodowski March 26, 2019, 3:46 p.m. UTC | #1
On Tue, Mar 26, 2019 at 01:39:21PM +0100, Aurelien Aptel wrote:
> Replace kref_t by a simple refcount. We do not care about the
> atomicity of the operation as long as the mutex is held.
> 
> This fixes a false-positive memory leak warning from kref_put() where
> we close the cached fid twice and make the kref underflow.
> 
> Link: https://lore.kernel.org/linux-cifs/20190319115151.GA2092@light.dominikbrodowski.net/
> 
> Signed-off-by: Aurelien Aptel <aaptel@suse.com>

Reported-and-tested-by: Dominik Brodowski <linux@dominikbrodowski.net>


Thanks,
	Dominik
Aurélien Aptel March 26, 2019, 4:53 p.m. UTC | #2
Dominik Brodowski <linux@dominikbrodowski.net> writes:
> Reported-and-tested-by: Dominik Brodowski <linux@dominikbrodowski.net>

Great, thanks Dominik.

I chose to keep a refcount_t instead of a simple int because it still checks
for overflows.
Dominik Brodowski March 26, 2019, 4:53 p.m. UTC | #3
On Tue, Mar 26, 2019 at 05:53:15PM +0100, Aurélien Aptel wrote:
> 
> Dominik Brodowski <linux@dominikbrodowski.net> writes:
> > Reported-and-tested-by: Dominik Brodowski <linux@dominikbrodowski.net>
> 
> Great, thanks Dominik.
> 
> I chose to keep a refcount_t instead of a simple int because it still checks
> for overflows.

Yes, that sounds to be the better option.

Thanks,
	Dominik
Pavel Shilovsky March 27, 2019, 10:36 p.m. UTC | #4
вт, 26 мар. 2019 г. в 05:39, Aurelien Aptel <aaptel@suse.com>:
>
> Replace kref_t by a simple refcount. We do not care about the
> atomicity of the operation as long as the mutex is held.
>
> This fixes a false-positive memory leak warning from kref_put() where
> we close the cached fid twice and make the kref underflow.
>
> Link: https://lore.kernel.org/linux-cifs/20190319115151.GA2092@light.dominikbrodowski.net/
>
> Signed-off-by: Aurelien Aptel <aaptel@suse.com>
> ---
>  fs/cifs/cifsglob.h |  2 +-
>  fs/cifs/smb2ops.c  | 34 ++++++++++++++--------------------
>  2 files changed, 15 insertions(+), 21 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 38feae812b47..256cd48fb4c7 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -972,7 +972,7 @@ struct cached_fid {
>         bool is_valid:1;        /* Do we have a useable root fid */
>         bool file_all_info_is_valid:1;
>
> -       struct kref refcount;
> +       refcount_t refcount;
>         struct cifs_fid *fid;
>         struct mutex fid_mutex;
>         struct cifs_tcon *tcon;
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 1022a3771e14..062c081a298c 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -608,25 +608,21 @@ SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon)
>         return rc;
>  }
>
> -static void
> -smb2_close_cached_fid(struct kref *ref)
> -{
> -       struct cached_fid *cfid = container_of(ref, struct cached_fid,
> -                                              refcount);
> -
> -       if (cfid->is_valid) {
> -               cifs_dbg(FYI, "clear cached root file handle\n");
> -               SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid,
> -                          cfid->fid->volatile_fid);
> -               cfid->is_valid = false;
> -               cfid->file_all_info_is_valid = false;
> -       }
> -}
> -
>  void close_shroot(struct cached_fid *cfid)
>  {
> +       unsigned int n;
>         mutex_lock(&cfid->fid_mutex);
> -       kref_put(&cfid->refcount, smb2_close_cached_fid);
> +       n = refcount_read(&cfid->refcount);
> +       if (n > 0) {
> +               refcount_dec(&cfid->refcount);
> +               if (n == 1 && cfid->is_valid) {
> +                       cifs_dbg(FYI, "clear cached root file handle\n");
> +                       SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid,
> +                                  cfid->fid->volatile_fid);
> +                       cfid->is_valid = false;
> +                       cfid->file_all_info_is_valid = false;
> +               }
> +       }
>         mutex_unlock(&cfid->fid_mutex);
>  }
>
> @@ -662,7 +658,7 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
>         if (tcon->crfid.is_valid) {
>                 cifs_dbg(FYI, "found a cached root file handle\n");
>                 memcpy(pfid, tcon->crfid.fid, sizeof(struct cifs_fid));
> -               kref_get(&tcon->crfid.refcount);
> +               refcount_inc(&tcon->crfid.refcount);
>                 mutex_unlock(&tcon->crfid.fid_mutex);
>                 return 0;
>         }
> @@ -728,9 +724,7 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
>         memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
>         tcon->crfid.tcon = tcon;
>         tcon->crfid.is_valid = true;
> -       kref_init(&tcon->crfid.refcount);
> -       kref_get(&tcon->crfid.refcount);
> -
> +       refcount_set(&tcon->crfid.refcount, 1);
>
>         qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
>         if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info))
> --
> 2.16.4
>

I looked at the usage of cached root handle: we call open_shroot in
two places (https://github.com/smfrench/smb3-kernel/search?q=open_shroot&unscoped_q=open_shroot):

1) smb3_qfs_tcon
2) smb2_query_path_info

In both places we call close_shroot() after we use the handle. Another
extra reference (that keeps the file handle opened) is being acquired
by open_shroot itself:

kref_init(&tcon->crfid.refcount);   <--- initialize to 1
kref_get(&tcon->crfid.refcount);   <--- bump to 2

So, once we stopped using the handle, there should be one reference
left. This reference is being put by the lease break handling code
when such a lease break comes. But here is the problem:

--------------------------------open_shroot()--------------------------------
rc = compound_send_recv(xid, ses, flags, 2, rqst,
                                            resp_buftype, rsp_iov);
if (rc)
    goto oshr_exit;

o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base;
oparms.fid->persistent_fid = o_rsp->PersistentFileId;
oparms.fid->volatile_fid = o_rsp->VolatileFileId;
#ifdef CONFIG_CIFS_DEBUG2
oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId);
#endif /* CIFS_DEBUG2 */

if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE)
    oplock = smb2_parse_lease_state(server, o_rsp,
                                                            &oparms.fid->epoch,

oparms.fid->lease_key);
else
    goto oshr_exit;
     ^^^
------------------------------------------------------------------------------------

if we the server doesn't respond with a lease, we return with rc=0
immediately without marking such a handle as cached and without
holding any reference to the handle (even the one that were
requested). Then we call close_shroot() trying to put non-existing
reference -- BUG that was reported.

The proper fix would be to continue initializing the cached root
handle but skip getting the extra reference (see kref_get call
explained above) if the server didn't give a lease. It doesn't seem
that any other changes are needed here.

--
Best regards,
Pavel Shilovsky
ronnie sahlberg March 27, 2019, 11:44 p.m. UTC | #5
On Thu, Mar 28, 2019 at 8:36 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> вт, 26 мар. 2019 г. в 05:39, Aurelien Aptel <aaptel@suse.com>:
> >
> > Replace kref_t by a simple refcount. We do not care about the
> > atomicity of the operation as long as the mutex is held.
> >
> > This fixes a false-positive memory leak warning from kref_put() where
> > we close the cached fid twice and make the kref underflow.
> >
> > Link: https://lore.kernel.org/linux-cifs/20190319115151.GA2092@light.dominikbrodowski.net/
> >
> > Signed-off-by: Aurelien Aptel <aaptel@suse.com>
> > ---
> >  fs/cifs/cifsglob.h |  2 +-
> >  fs/cifs/smb2ops.c  | 34 ++++++++++++++--------------------
> >  2 files changed, 15 insertions(+), 21 deletions(-)
> >
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > index 38feae812b47..256cd48fb4c7 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -972,7 +972,7 @@ struct cached_fid {
> >         bool is_valid:1;        /* Do we have a useable root fid */
> >         bool file_all_info_is_valid:1;
> >
> > -       struct kref refcount;
> > +       refcount_t refcount;
> >         struct cifs_fid *fid;
> >         struct mutex fid_mutex;
> >         struct cifs_tcon *tcon;
> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > index 1022a3771e14..062c081a298c 100644
> > --- a/fs/cifs/smb2ops.c
> > +++ b/fs/cifs/smb2ops.c
> > @@ -608,25 +608,21 @@ SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon)
> >         return rc;
> >  }
> >
> > -static void
> > -smb2_close_cached_fid(struct kref *ref)
> > -{
> > -       struct cached_fid *cfid = container_of(ref, struct cached_fid,
> > -                                              refcount);
> > -
> > -       if (cfid->is_valid) {
> > -               cifs_dbg(FYI, "clear cached root file handle\n");
> > -               SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid,
> > -                          cfid->fid->volatile_fid);
> > -               cfid->is_valid = false;
> > -               cfid->file_all_info_is_valid = false;
> > -       }
> > -}
> > -
> >  void close_shroot(struct cached_fid *cfid)
> >  {
> > +       unsigned int n;
> >         mutex_lock(&cfid->fid_mutex);
> > -       kref_put(&cfid->refcount, smb2_close_cached_fid);
> > +       n = refcount_read(&cfid->refcount);
> > +       if (n > 0) {
> > +               refcount_dec(&cfid->refcount);
> > +               if (n == 1 && cfid->is_valid) {
> > +                       cifs_dbg(FYI, "clear cached root file handle\n");
> > +                       SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid,
> > +                                  cfid->fid->volatile_fid);
> > +                       cfid->is_valid = false;
> > +                       cfid->file_all_info_is_valid = false;
> > +               }
> > +       }
> >         mutex_unlock(&cfid->fid_mutex);
> >  }
> >
> > @@ -662,7 +658,7 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
> >         if (tcon->crfid.is_valid) {
> >                 cifs_dbg(FYI, "found a cached root file handle\n");
> >                 memcpy(pfid, tcon->crfid.fid, sizeof(struct cifs_fid));
> > -               kref_get(&tcon->crfid.refcount);
> > +               refcount_inc(&tcon->crfid.refcount);
> >                 mutex_unlock(&tcon->crfid.fid_mutex);
> >                 return 0;
> >         }
> > @@ -728,9 +724,7 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
> >         memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
> >         tcon->crfid.tcon = tcon;
> >         tcon->crfid.is_valid = true;
> > -       kref_init(&tcon->crfid.refcount);
> > -       kref_get(&tcon->crfid.refcount);
> > -
> > +       refcount_set(&tcon->crfid.refcount, 1);
> >
> >         qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
> >         if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info))
> > --
> > 2.16.4
> >
>
> I looked at the usage of cached root handle: we call open_shroot in
> two places (https://github.com/smfrench/smb3-kernel/search?q=open_shroot&unscoped_q=open_shroot):
>
> 1) smb3_qfs_tcon
> 2) smb2_query_path_info
>
> In both places we call close_shroot() after we use the handle. Another
> extra reference (that keeps the file handle opened) is being acquired
> by open_shroot itself:
>
> kref_init(&tcon->crfid.refcount);   <--- initialize to 1
> kref_get(&tcon->crfid.refcount);   <--- bump to 2
>
> So, once we stopped using the handle, there should be one reference
> left. This reference is being put by the lease break handling code
> when such a lease break comes. But here is the problem:
>
> --------------------------------open_shroot()--------------------------------
> rc = compound_send_recv(xid, ses, flags, 2, rqst,
>                                             resp_buftype, rsp_iov);
> if (rc)
>     goto oshr_exit;
>
> o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base;
> oparms.fid->persistent_fid = o_rsp->PersistentFileId;
> oparms.fid->volatile_fid = o_rsp->VolatileFileId;
> #ifdef CONFIG_CIFS_DEBUG2
> oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId);
> #endif /* CIFS_DEBUG2 */
>
> if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE)
>     oplock = smb2_parse_lease_state(server, o_rsp,
>                                                             &oparms.fid->epoch,
>
> oparms.fid->lease_key);
> else
>     goto oshr_exit;
>      ^^^
> ------------------------------------------------------------------------------------
>
> if we the server doesn't respond with a lease, we return with rc=0
> immediately without marking such a handle as cached and without
> holding any reference to the handle (even the one that were
> requested). Then we call close_shroot() trying to put non-existing
> reference -- BUG that was reported.
>
> The proper fix would be to continue initializing the cached root
> handle but skip getting the extra reference (see kref_get call
> explained above) if the server didn't give a lease. It doesn't seem
> that any other changes are needed here.

Good analysis. I think you are right.
I would actually also move this initialization so it happens in
oshr_exit: and make it conditional to rc==0.
I will send a patch for this.

Thanks.

>
> --
> Best regards,
> Pavel Shilovsky
diff mbox series

Patch

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 38feae812b47..256cd48fb4c7 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -972,7 +972,7 @@  struct cached_fid {
 	bool is_valid:1;	/* Do we have a useable root fid */
 	bool file_all_info_is_valid:1;
 
-	struct kref refcount;
+	refcount_t refcount;
 	struct cifs_fid *fid;
 	struct mutex fid_mutex;
 	struct cifs_tcon *tcon;
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 1022a3771e14..062c081a298c 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -608,25 +608,21 @@  SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon)
 	return rc;
 }
 
-static void
-smb2_close_cached_fid(struct kref *ref)
-{
-	struct cached_fid *cfid = container_of(ref, struct cached_fid,
-					       refcount);
-
-	if (cfid->is_valid) {
-		cifs_dbg(FYI, "clear cached root file handle\n");
-		SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid,
-			   cfid->fid->volatile_fid);
-		cfid->is_valid = false;
-		cfid->file_all_info_is_valid = false;
-	}
-}
-
 void close_shroot(struct cached_fid *cfid)
 {
+	unsigned int n;
 	mutex_lock(&cfid->fid_mutex);
-	kref_put(&cfid->refcount, smb2_close_cached_fid);
+	n = refcount_read(&cfid->refcount);
+	if (n > 0) {
+		refcount_dec(&cfid->refcount);
+		if (n == 1 && cfid->is_valid) {
+			cifs_dbg(FYI, "clear cached root file handle\n");
+			SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid,
+				   cfid->fid->volatile_fid);
+			cfid->is_valid = false;
+			cfid->file_all_info_is_valid = false;
+		}
+	}
 	mutex_unlock(&cfid->fid_mutex);
 }
 
@@ -662,7 +658,7 @@  int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
 	if (tcon->crfid.is_valid) {
 		cifs_dbg(FYI, "found a cached root file handle\n");
 		memcpy(pfid, tcon->crfid.fid, sizeof(struct cifs_fid));
-		kref_get(&tcon->crfid.refcount);
+		refcount_inc(&tcon->crfid.refcount);
 		mutex_unlock(&tcon->crfid.fid_mutex);
 		return 0;
 	}
@@ -728,9 +724,7 @@  int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
 	memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
 	tcon->crfid.tcon = tcon;
 	tcon->crfid.is_valid = true;
-	kref_init(&tcon->crfid.refcount);
-	kref_get(&tcon->crfid.refcount);
-
+	refcount_set(&tcon->crfid.refcount, 1);
 
 	qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
 	if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info))