On Thu, Mar 28, 2019 at 8:36 AM Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote: > > вт, 26 мар. 2019 г. в 05:39, Aurelien Aptel <aaptel@xxxxxxxx>: > > > > 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@xxxxxxxxxxxxxxxxxxxxxxxxxx/ > > > > Signed-off-by: Aurelien Aptel <aaptel@xxxxxxxx> > > --- > > 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
