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