вт, 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. -- Best regards, Pavel Shilovsky
