| Message ID | 20250502180136.192407-1-henrique.carvalho@suse.com |
|---|---|
| State | New |
| Headers | show |
| Series | smb: cached_dir.c: fix race in cfid release | expand |
I fixed a minor checkpatch warning but also noticed this compile warning - is there a missing lock call? cached_dir.c:429:20: warning: context imbalance in 'cfid_release' - unexpected unlock On Fri, May 2, 2025 at 1:04 PM Henrique Carvalho <henrique.carvalho@suse.com> wrote: > > find_or_create_cached_dir() could grab a new reference after kref_put() > had seen the refcount drop to zero but before cfid_list_lock is acquired > in smb2_close_cached_fid(), leading to use-after-free. > > Switch to kref_put_lock() so cfid_release() runs with cfid_list_lock > held, closing that gap. > > While we are at it: > * rename the functions smb2_close_cached_fid() and close_cached_dir() > for clarity; > * replace the calls to kref_put() for cfid_put(). > > Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com> > --- > fs/smb/client/cached_dir.c | 28 ++++++++++++++++------------ > fs/smb/client/cached_dir.h | 2 +- > fs/smb/client/inode.c | 4 ++-- > fs/smb/client/readdir.c | 4 ++-- > fs/smb/client/smb2inode.c | 2 +- > fs/smb/client/smb2ops.c | 8 ++++---- > 6 files changed, 26 insertions(+), 22 deletions(-) > > diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c > index fe738623cf1b..ff4f9f8150cf 100644 > --- a/fs/smb/client/cached_dir.c > +++ b/fs/smb/client/cached_dir.c > @@ -14,7 +14,7 @@ > > static struct cached_fid *init_cached_dir(const char *path); > static void free_cached_dir(struct cached_fid *cfid); > -static void smb2_close_cached_fid(struct kref *ref); > +static void cfid_release(struct kref *ref); > static void cfids_laundromat_worker(struct work_struct *work); > > struct cached_dir_dentry { > @@ -370,11 +370,13 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, > * lease. Release one here, and the second below. > */ > cfid->has_lease = false; > - kref_put(&cfid->refcount, smb2_close_cached_fid); > + > + /* If this cfid_put calls back cfid_release the code is wrong anyway. */ > + cfid_put(cfid); > } > spin_unlock(&cfids->cfid_list_lock); > > - kref_put(&cfid->refcount, smb2_close_cached_fid); > + cfid_put(cfid); > } else { > *ret_cfid = cfid; > atomic_inc(&tcon->num_remote_opens); > @@ -413,13 +415,12 @@ int open_cached_dir_by_dentry(struct cifs_tcon *tcon, > } > > static void > -smb2_close_cached_fid(struct kref *ref) > +cfid_release(struct kref *ref) > { > struct cached_fid *cfid = container_of(ref, struct cached_fid, > refcount); > int rc; > > - spin_lock(&cfid->cfids->cfid_list_lock); > if (cfid->on_list) { > list_del(&cfid->entry); > cfid->on_list = false; > @@ -454,16 +455,19 @@ void drop_cached_dir_by_name(const unsigned int xid, struct cifs_tcon *tcon, > spin_lock(&cfid->cfids->cfid_list_lock); > if (cfid->has_lease) { > cfid->has_lease = false; > - kref_put(&cfid->refcount, smb2_close_cached_fid); > + > + /* If this cfid_put calls back cfid_release the code is wrong anyway. */ > + cfid_put(cfid); > } > spin_unlock(&cfid->cfids->cfid_list_lock); > - close_cached_dir(cfid); > + cfid_put(cfid); > } > > > -void close_cached_dir(struct cached_fid *cfid) > +void cfid_put(struct cached_fid *cfid) > { > - kref_put(&cfid->refcount, smb2_close_cached_fid); > + struct cached_fids *cfids = cfid->tcon->cfids; > + kref_put_lock(&cfid->refcount, cfid_release, &cfids->cfid_list_lock); > } > > /* > @@ -564,7 +568,7 @@ cached_dir_offload_close(struct work_struct *work) > > WARN_ON(cfid->on_list); > > - kref_put(&cfid->refcount, smb2_close_cached_fid); > + cfid_put(cfid); > cifs_put_tcon(tcon, netfs_trace_tcon_ref_put_cached_close); > } > > @@ -688,7 +692,7 @@ static void cfids_invalidation_worker(struct work_struct *work) > list_for_each_entry_safe(cfid, q, &entry, entry) { > list_del(&cfid->entry); > /* Drop the ref-count acquired in invalidate_all_cached_dirs */ > - kref_put(&cfid->refcount, smb2_close_cached_fid); > + cfid_put(cfid); > } > } > > @@ -741,7 +745,7 @@ static void cfids_laundromat_worker(struct work_struct *work) > * Drop the ref-count from above, either the lease-ref (if there > * was one) or the extra one acquired. > */ > - kref_put(&cfid->refcount, smb2_close_cached_fid); > + cfid_put(cfid); > } > queue_delayed_work(cfid_put_wq, &cfids->laundromat_work, > dir_cache_timeout * HZ); > diff --git a/fs/smb/client/cached_dir.h b/fs/smb/client/cached_dir.h > index 1dfe79d947a6..f4fc7818dda5 100644 > --- a/fs/smb/client/cached_dir.h > +++ b/fs/smb/client/cached_dir.h > @@ -73,7 +73,7 @@ extern int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, > extern int open_cached_dir_by_dentry(struct cifs_tcon *tcon, > struct dentry *dentry, > struct cached_fid **cfid); > -extern void close_cached_dir(struct cached_fid *cfid); > +extern void cfid_put(struct cached_fid *cfid); > extern void drop_cached_dir_by_name(const unsigned int xid, > struct cifs_tcon *tcon, > const char *name, > diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c > index 75be4b46bc6f..c3ef1f93a80d 100644 > --- a/fs/smb/client/inode.c > +++ b/fs/smb/client/inode.c > @@ -2610,10 +2610,10 @@ cifs_dentry_needs_reval(struct dentry *dentry) > > if (!open_cached_dir_by_dentry(tcon, dentry->d_parent, &cfid)) { > if (cfid->time && cifs_i->time > cfid->time) { > - close_cached_dir(cfid); > + cfid_put(cfid); > return false; > } > - close_cached_dir(cfid); > + cfid_put(cfid); > } > /* > * depending on inode type, check if attribute caching disabled for > diff --git a/fs/smb/client/readdir.c b/fs/smb/client/readdir.c > index 50f96259d9ad..1e1768152803 100644 > --- a/fs/smb/client/readdir.c > +++ b/fs/smb/client/readdir.c > @@ -1093,7 +1093,7 @@ int cifs_readdir(struct file *file, struct dir_context *ctx) > * find_cifs_entry in case there will be reconnects during > * query_directory. > */ > - close_cached_dir(cfid); > + cfid_put(cfid); > cfid = NULL; > > cache_not_found: > @@ -1199,7 +1199,7 @@ int cifs_readdir(struct file *file, struct dir_context *ctx) > > rddir2_exit: > if (cfid) > - close_cached_dir(cfid); > + cfid_put(cfid); > free_dentry_path(page); > free_xid(xid); > return rc; > diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c > index 57d9bfbadd97..f805d71a8d19 100644 > --- a/fs/smb/client/smb2inode.c > +++ b/fs/smb/client/smb2inode.c > @@ -975,7 +975,7 @@ int smb2_query_path_info(const unsigned int xid, > cfid->fid.volatile_fid, > &data->fi); > } > - close_cached_dir(cfid); > + cfid_put(cfid); > return rc; > } > cmds[num_cmds++] = SMB2_OP_QUERY_INFO; > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c > index 2fe8eeb98535..97c4d44c9a69 100644 > --- a/fs/smb/client/smb2ops.c > +++ b/fs/smb/client/smb2ops.c > @@ -889,7 +889,7 @@ smb3_qfs_tcon(const unsigned int xid, struct cifs_tcon *tcon, > if (cfid == NULL) > SMB2_close(xid, tcon, fid.persistent_fid, fid.volatile_fid); > else > - close_cached_dir(cfid); > + cfid_put(cfid); > } > > static void > @@ -940,10 +940,10 @@ smb2_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon, > rc = open_cached_dir(xid, tcon, full_path, cifs_sb, true, &cfid); > if (!rc) { > if (cfid->has_lease) { > - close_cached_dir(cfid); > + cfid_put(cfid); > return 0; > } > - close_cached_dir(cfid); > + cfid_put(cfid); > } > > utf16_path = cifs_convert_path_to_utf16(full_path, cifs_sb); > @@ -2804,7 +2804,7 @@ smb2_query_info_compound(const unsigned int xid, struct cifs_tcon *tcon, > free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base); > free_rsp_buf(resp_buftype[2], rsp_iov[2].iov_base); > if (cfid) > - close_cached_dir(cfid); > + cfid_put(cfid); > kfree(vars); > out_free_path: > kfree(utf16_path); > -- > 2.47.0 > >
On 05/02, Steve French wrote: >I fixed a minor checkpatch warning but also noticed this compile >warning - is there a missing lock call? > >cached_dir.c:429:20: warning: context imbalance in 'cfid_release' - >unexpected unlock The lock is taken (inside kref_put_lock) if count == 0 (i.e. when the release function is called) and must be released from within the release function (which is done here). However, sparse can't recognize this and also there doesn't seem to exist an annotation to indicate so. @Henrique do you think you could rework the patch to something like: cfid_release() { list_del(); on_list = false; num_entries--; } cfid_put() { lock(); if (kref_put(..., cfid_release)) { unlock(); dput(); SMB2_close(); free_cached_dir(); return; } unlock(); } Cheers, Enzo >On Fri, May 2, 2025 at 1:04 PM Henrique Carvalho ><henrique.carvalho@suse.com> wrote: >> >> find_or_create_cached_dir() could grab a new reference after kref_put() >> had seen the refcount drop to zero but before cfid_list_lock is acquired >> in smb2_close_cached_fid(), leading to use-after-free. >> >> Switch to kref_put_lock() so cfid_release() runs with cfid_list_lock >> held, closing that gap. >> >> While we are at it: >> * rename the functions smb2_close_cached_fid() and close_cached_dir() >> for clarity; >> * replace the calls to kref_put() for cfid_put(). >> >> Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com> >> --- >> fs/smb/client/cached_dir.c | 28 ++++++++++++++++------------ >> fs/smb/client/cached_dir.h | 2 +- >> fs/smb/client/inode.c | 4 ++-- >> fs/smb/client/readdir.c | 4 ++-- >> fs/smb/client/smb2inode.c | 2 +- >> fs/smb/client/smb2ops.c | 8 ++++---- >> 6 files changed, 26 insertions(+), 22 deletions(-) >> >> diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c >> index fe738623cf1b..ff4f9f8150cf 100644 >> --- a/fs/smb/client/cached_dir.c >> +++ b/fs/smb/client/cached_dir.c >> @@ -14,7 +14,7 @@ >> >> static struct cached_fid *init_cached_dir(const char *path); >> static void free_cached_dir(struct cached_fid *cfid); >> -static void smb2_close_cached_fid(struct kref *ref); >> +static void cfid_release(struct kref *ref); >> static void cfids_laundromat_worker(struct work_struct *work); >> >> struct cached_dir_dentry { >> @@ -370,11 +370,13 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, >> * lease. Release one here, and the second below. >> */ >> cfid->has_lease = false; >> - kref_put(&cfid->refcount, smb2_close_cached_fid); >> + >> + /* If this cfid_put calls back cfid_release the code is wrong anyway. */ >> + cfid_put(cfid); >> } >> spin_unlock(&cfids->cfid_list_lock); >> >> - kref_put(&cfid->refcount, smb2_close_cached_fid); >> + cfid_put(cfid); >> } else { >> *ret_cfid = cfid; >> atomic_inc(&tcon->num_remote_opens); >> @@ -413,13 +415,12 @@ int open_cached_dir_by_dentry(struct cifs_tcon *tcon, >> } >> >> static void >> -smb2_close_cached_fid(struct kref *ref) >> +cfid_release(struct kref *ref) >> { >> struct cached_fid *cfid = container_of(ref, struct cached_fid, >> refcount); >> int rc; >> >> - spin_lock(&cfid->cfids->cfid_list_lock); >> if (cfid->on_list) { >> list_del(&cfid->entry); >> cfid->on_list = false; >> @@ -454,16 +455,19 @@ void drop_cached_dir_by_name(const unsigned int xid, struct cifs_tcon *tcon, >> spin_lock(&cfid->cfids->cfid_list_lock); >> if (cfid->has_lease) { >> cfid->has_lease = false; >> - kref_put(&cfid->refcount, smb2_close_cached_fid); >> + >> + /* If this cfid_put calls back cfid_release the code is wrong anyway. */ >> + cfid_put(cfid); >> } >> spin_unlock(&cfid->cfids->cfid_list_lock); >> - close_cached_dir(cfid); >> + cfid_put(cfid); >> } >> >> >> -void close_cached_dir(struct cached_fid *cfid) >> +void cfid_put(struct cached_fid *cfid) >> { >> - kref_put(&cfid->refcount, smb2_close_cached_fid); >> + struct cached_fids *cfids = cfid->tcon->cfids; >> + kref_put_lock(&cfid->refcount, cfid_release, &cfids->cfid_list_lock); >> } >> >> /* >> @@ -564,7 +568,7 @@ cached_dir_offload_close(struct work_struct *work) >> >> WARN_ON(cfid->on_list); >> >> - kref_put(&cfid->refcount, smb2_close_cached_fid); >> + cfid_put(cfid); >> cifs_put_tcon(tcon, netfs_trace_tcon_ref_put_cached_close); >> } >> >> @@ -688,7 +692,7 @@ static void cfids_invalidation_worker(struct work_struct *work) >> list_for_each_entry_safe(cfid, q, &entry, entry) { >> list_del(&cfid->entry); >> /* Drop the ref-count acquired in invalidate_all_cached_dirs */ >> - kref_put(&cfid->refcount, smb2_close_cached_fid); >> + cfid_put(cfid); >> } >> } >> >> @@ -741,7 +745,7 @@ static void cfids_laundromat_worker(struct work_struct *work) >> * Drop the ref-count from above, either the lease-ref (if there >> * was one) or the extra one acquired. >> */ >> - kref_put(&cfid->refcount, smb2_close_cached_fid); >> + cfid_put(cfid); >> } >> queue_delayed_work(cfid_put_wq, &cfids->laundromat_work, >> dir_cache_timeout * HZ); >> diff --git a/fs/smb/client/cached_dir.h b/fs/smb/client/cached_dir.h >> index 1dfe79d947a6..f4fc7818dda5 100644 >> --- a/fs/smb/client/cached_dir.h >> +++ b/fs/smb/client/cached_dir.h >> @@ -73,7 +73,7 @@ extern int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, >> extern int open_cached_dir_by_dentry(struct cifs_tcon *tcon, >> struct dentry *dentry, >> struct cached_fid **cfid); >> -extern void close_cached_dir(struct cached_fid *cfid); >> +extern void cfid_put(struct cached_fid *cfid); >> extern void drop_cached_dir_by_name(const unsigned int xid, >> struct cifs_tcon *tcon, >> const char *name, >> diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c >> index 75be4b46bc6f..c3ef1f93a80d 100644 >> --- a/fs/smb/client/inode.c >> +++ b/fs/smb/client/inode.c >> @@ -2610,10 +2610,10 @@ cifs_dentry_needs_reval(struct dentry *dentry) >> >> if (!open_cached_dir_by_dentry(tcon, dentry->d_parent, &cfid)) { >> if (cfid->time && cifs_i->time > cfid->time) { >> - close_cached_dir(cfid); >> + cfid_put(cfid); >> return false; >> } >> - close_cached_dir(cfid); >> + cfid_put(cfid); >> } >> /* >> * depending on inode type, check if attribute caching disabled for >> diff --git a/fs/smb/client/readdir.c b/fs/smb/client/readdir.c >> index 50f96259d9ad..1e1768152803 100644 >> --- a/fs/smb/client/readdir.c >> +++ b/fs/smb/client/readdir.c >> @@ -1093,7 +1093,7 @@ int cifs_readdir(struct file *file, struct dir_context *ctx) >> * find_cifs_entry in case there will be reconnects during >> * query_directory. >> */ >> - close_cached_dir(cfid); >> + cfid_put(cfid); >> cfid = NULL; >> >> cache_not_found: >> @@ -1199,7 +1199,7 @@ int cifs_readdir(struct file *file, struct dir_context *ctx) >> >> rddir2_exit: >> if (cfid) >> - close_cached_dir(cfid); >> + cfid_put(cfid); >> free_dentry_path(page); >> free_xid(xid); >> return rc; >> diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c >> index 57d9bfbadd97..f805d71a8d19 100644 >> --- a/fs/smb/client/smb2inode.c >> +++ b/fs/smb/client/smb2inode.c >> @@ -975,7 +975,7 @@ int smb2_query_path_info(const unsigned int xid, >> cfid->fid.volatile_fid, >> &data->fi); >> } >> - close_cached_dir(cfid); >> + cfid_put(cfid); >> return rc; >> } >> cmds[num_cmds++] = SMB2_OP_QUERY_INFO; >> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c >> index 2fe8eeb98535..97c4d44c9a69 100644 >> --- a/fs/smb/client/smb2ops.c >> +++ b/fs/smb/client/smb2ops.c >> @@ -889,7 +889,7 @@ smb3_qfs_tcon(const unsigned int xid, struct cifs_tcon *tcon, >> if (cfid == NULL) >> SMB2_close(xid, tcon, fid.persistent_fid, fid.volatile_fid); >> else >> - close_cached_dir(cfid); >> + cfid_put(cfid); >> } >> >> static void >> @@ -940,10 +940,10 @@ smb2_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon, >> rc = open_cached_dir(xid, tcon, full_path, cifs_sb, true, &cfid); >> if (!rc) { >> if (cfid->has_lease) { >> - close_cached_dir(cfid); >> + cfid_put(cfid); >> return 0; >> } >> - close_cached_dir(cfid); >> + cfid_put(cfid); >> } >> >> utf16_path = cifs_convert_path_to_utf16(full_path, cifs_sb); >> @@ -2804,7 +2804,7 @@ smb2_query_info_compound(const unsigned int xid, struct cifs_tcon *tcon, >> free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base); >> free_rsp_buf(resp_buftype[2], rsp_iov[2].iov_base); >> if (cfid) >> - close_cached_dir(cfid); >> + cfid_put(cfid); >> kfree(vars); >> out_free_path: >> kfree(utf16_path); >> -- >> 2.47.0 >> >> > > >-- >Thanks, > >Steve >
On Fri, May 02, 2025 at 04:58:00PM -0300, Enzo Matsumiya wrote: > On 05/02, Steve French wrote: > > I fixed a minor checkpatch warning but also noticed this compile > > warning - is there a missing lock call? > > > > cached_dir.c:429:20: warning: context imbalance in 'cfid_release' - > > unexpected unlock > > The lock is taken (inside kref_put_lock) if count == 0 (i.e. when the > release function is called) and must be released from within the > release function (which is done here). > > However, sparse can't recognize this and also there doesn't seem to > exist an annotation to indicate so. > > @Henrique do you think you could rework the patch to something like: > > cfid_release() { > list_del(); > on_list = false; > num_entries--; > } > > cfid_put() { > lock(); > if (kref_put(..., cfid_release)) { > unlock(); > dput(); > SMB2_close(); > free_cached_dir(); > return; > } > unlock(); > } > @Enzo, good idea. I will rework the patch. Henrique
On Fri, May 02, 2025 at 05:20:31PM -0300, Henrique Carvalho wrote: > On Fri, May 02, 2025 at 04:58:00PM -0300, Enzo Matsumiya wrote: > > On 05/02, Steve French wrote: > > > I fixed a minor checkpatch warning but also noticed this compile > > > warning - is there a missing lock call? > > > > > > cached_dir.c:429:20: warning: context imbalance in 'cfid_release' - > > > unexpected unlock > > > > The lock is taken (inside kref_put_lock) if count == 0 (i.e. when the > > release function is called) and must be released from within the > > release function (which is done here). > > > > However, sparse can't recognize this and also there doesn't seem to > > exist an annotation to indicate so. > > > > @Henrique do you think you could rework the patch to something like: > > > > cfid_release() { > > list_del(); > > on_list = false; > > num_entries--; > > } > > > > cfid_put() { > > lock(); > > if (kref_put(..., cfid_release)) { > > unlock(); > > dput(); > > SMB2_close(); > > free_cached_dir(); > > return; > > } > > unlock(); > > } > > > > @Enzo, good idea. I will rework the patch. > Actually, this change would prevent me from calling cfid_put() with the lock held in cases where the kref does *not* reach 0 and the release function isn't supposed to run. While it could work, the code won't be as elegant. I’m open to suggestions if there's a way to preserve that behavior while satisfying sparse. In the meantime, I'm reviewing similar discussions on other mailing lists to see if there are known solutions. Henrique
The patch does work around the hang/crash after generic/241 but a few other tests failed unexpectedly (could be unrelated bugs) to windows multichannel http://smb311-linux-testing.southcentralus.cloudapp.azure.com/#/builders/12/builds/32 On Fri, May 2, 2025 at 4:01 PM Henrique Carvalho <henrique.carvalho@suse.com> wrote: > > On Fri, May 02, 2025 at 05:20:31PM -0300, Henrique Carvalho wrote: > > On Fri, May 02, 2025 at 04:58:00PM -0300, Enzo Matsumiya wrote: > > > On 05/02, Steve French wrote: > > > > I fixed a minor checkpatch warning but also noticed this compile > > > > warning - is there a missing lock call? > > > > > > > > cached_dir.c:429:20: warning: context imbalance in 'cfid_release' - > > > > unexpected unlock > > > > > > The lock is taken (inside kref_put_lock) if count == 0 (i.e. when the > > > release function is called) and must be released from within the > > > release function (which is done here). > > > > > > However, sparse can't recognize this and also there doesn't seem to > > > exist an annotation to indicate so. > > > > > > @Henrique do you think you could rework the patch to something like: > > > > > > cfid_release() { > > > list_del(); > > > on_list = false; > > > num_entries--; > > > } > > > > > > cfid_put() { > > > lock(); > > > if (kref_put(..., cfid_release)) { > > > unlock(); > > > dput(); > > > SMB2_close(); > > > free_cached_dir(); > > > return; > > > } > > > unlock(); > > > } > > > > > > > @Enzo, good idea. I will rework the patch. > > > > Actually, this change would prevent me from calling cfid_put() with the > lock held in cases where the kref does *not* reach 0 and the release > function isn't supposed to run. While it could work, the code won't be > as elegant. > > I’m open to suggestions if there's a way to preserve that behavior > while satisfying sparse. > > In the meantime, I'm reviewing similar discussions on other mailing > lists to see if there are known solutions. > > > Henrique
Using __releases() annotation seemed to work, I am sending a second version of the patch. Henrique
diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c index fe738623cf1b..ff4f9f8150cf 100644 --- a/fs/smb/client/cached_dir.c +++ b/fs/smb/client/cached_dir.c @@ -14,7 +14,7 @@ static struct cached_fid *init_cached_dir(const char *path); static void free_cached_dir(struct cached_fid *cfid); -static void smb2_close_cached_fid(struct kref *ref); +static void cfid_release(struct kref *ref); static void cfids_laundromat_worker(struct work_struct *work); struct cached_dir_dentry { @@ -370,11 +370,13 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, * lease. Release one here, and the second below. */ cfid->has_lease = false; - kref_put(&cfid->refcount, smb2_close_cached_fid); + + /* If this cfid_put calls back cfid_release the code is wrong anyway. */ + cfid_put(cfid); } spin_unlock(&cfids->cfid_list_lock); - kref_put(&cfid->refcount, smb2_close_cached_fid); + cfid_put(cfid); } else { *ret_cfid = cfid; atomic_inc(&tcon->num_remote_opens); @@ -413,13 +415,12 @@ int open_cached_dir_by_dentry(struct cifs_tcon *tcon, } static void -smb2_close_cached_fid(struct kref *ref) +cfid_release(struct kref *ref) { struct cached_fid *cfid = container_of(ref, struct cached_fid, refcount); int rc; - spin_lock(&cfid->cfids->cfid_list_lock); if (cfid->on_list) { list_del(&cfid->entry); cfid->on_list = false; @@ -454,16 +455,19 @@ void drop_cached_dir_by_name(const unsigned int xid, struct cifs_tcon *tcon, spin_lock(&cfid->cfids->cfid_list_lock); if (cfid->has_lease) { cfid->has_lease = false; - kref_put(&cfid->refcount, smb2_close_cached_fid); + + /* If this cfid_put calls back cfid_release the code is wrong anyway. */ + cfid_put(cfid); } spin_unlock(&cfid->cfids->cfid_list_lock); - close_cached_dir(cfid); + cfid_put(cfid); } -void close_cached_dir(struct cached_fid *cfid) +void cfid_put(struct cached_fid *cfid) { - kref_put(&cfid->refcount, smb2_close_cached_fid); + struct cached_fids *cfids = cfid->tcon->cfids; + kref_put_lock(&cfid->refcount, cfid_release, &cfids->cfid_list_lock); } /* @@ -564,7 +568,7 @@ cached_dir_offload_close(struct work_struct *work) WARN_ON(cfid->on_list); - kref_put(&cfid->refcount, smb2_close_cached_fid); + cfid_put(cfid); cifs_put_tcon(tcon, netfs_trace_tcon_ref_put_cached_close); } @@ -688,7 +692,7 @@ static void cfids_invalidation_worker(struct work_struct *work) list_for_each_entry_safe(cfid, q, &entry, entry) { list_del(&cfid->entry); /* Drop the ref-count acquired in invalidate_all_cached_dirs */ - kref_put(&cfid->refcount, smb2_close_cached_fid); + cfid_put(cfid); } } @@ -741,7 +745,7 @@ static void cfids_laundromat_worker(struct work_struct *work) * Drop the ref-count from above, either the lease-ref (if there * was one) or the extra one acquired. */ - kref_put(&cfid->refcount, smb2_close_cached_fid); + cfid_put(cfid); } queue_delayed_work(cfid_put_wq, &cfids->laundromat_work, dir_cache_timeout * HZ); diff --git a/fs/smb/client/cached_dir.h b/fs/smb/client/cached_dir.h index 1dfe79d947a6..f4fc7818dda5 100644 --- a/fs/smb/client/cached_dir.h +++ b/fs/smb/client/cached_dir.h @@ -73,7 +73,7 @@ extern int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, extern int open_cached_dir_by_dentry(struct cifs_tcon *tcon, struct dentry *dentry, struct cached_fid **cfid); -extern void close_cached_dir(struct cached_fid *cfid); +extern void cfid_put(struct cached_fid *cfid); extern void drop_cached_dir_by_name(const unsigned int xid, struct cifs_tcon *tcon, const char *name, diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c index 75be4b46bc6f..c3ef1f93a80d 100644 --- a/fs/smb/client/inode.c +++ b/fs/smb/client/inode.c @@ -2610,10 +2610,10 @@ cifs_dentry_needs_reval(struct dentry *dentry) if (!open_cached_dir_by_dentry(tcon, dentry->d_parent, &cfid)) { if (cfid->time && cifs_i->time > cfid->time) { - close_cached_dir(cfid); + cfid_put(cfid); return false; } - close_cached_dir(cfid); + cfid_put(cfid); } /* * depending on inode type, check if attribute caching disabled for diff --git a/fs/smb/client/readdir.c b/fs/smb/client/readdir.c index 50f96259d9ad..1e1768152803 100644 --- a/fs/smb/client/readdir.c +++ b/fs/smb/client/readdir.c @@ -1093,7 +1093,7 @@ int cifs_readdir(struct file *file, struct dir_context *ctx) * find_cifs_entry in case there will be reconnects during * query_directory. */ - close_cached_dir(cfid); + cfid_put(cfid); cfid = NULL; cache_not_found: @@ -1199,7 +1199,7 @@ int cifs_readdir(struct file *file, struct dir_context *ctx) rddir2_exit: if (cfid) - close_cached_dir(cfid); + cfid_put(cfid); free_dentry_path(page); free_xid(xid); return rc; diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c index 57d9bfbadd97..f805d71a8d19 100644 --- a/fs/smb/client/smb2inode.c +++ b/fs/smb/client/smb2inode.c @@ -975,7 +975,7 @@ int smb2_query_path_info(const unsigned int xid, cfid->fid.volatile_fid, &data->fi); } - close_cached_dir(cfid); + cfid_put(cfid); return rc; } cmds[num_cmds++] = SMB2_OP_QUERY_INFO; diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c index 2fe8eeb98535..97c4d44c9a69 100644 --- a/fs/smb/client/smb2ops.c +++ b/fs/smb/client/smb2ops.c @@ -889,7 +889,7 @@ smb3_qfs_tcon(const unsigned int xid, struct cifs_tcon *tcon, if (cfid == NULL) SMB2_close(xid, tcon, fid.persistent_fid, fid.volatile_fid); else - close_cached_dir(cfid); + cfid_put(cfid); } static void @@ -940,10 +940,10 @@ smb2_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon, rc = open_cached_dir(xid, tcon, full_path, cifs_sb, true, &cfid); if (!rc) { if (cfid->has_lease) { - close_cached_dir(cfid); + cfid_put(cfid); return 0; } - close_cached_dir(cfid); + cfid_put(cfid); } utf16_path = cifs_convert_path_to_utf16(full_path, cifs_sb); @@ -2804,7 +2804,7 @@ smb2_query_info_compound(const unsigned int xid, struct cifs_tcon *tcon, free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base); free_rsp_buf(resp_buftype[2], rsp_iov[2].iov_base); if (cfid) - close_cached_dir(cfid); + cfid_put(cfid); kfree(vars); out_free_path: kfree(utf16_path);
find_or_create_cached_dir() could grab a new reference after kref_put() had seen the refcount drop to zero but before cfid_list_lock is acquired in smb2_close_cached_fid(), leading to use-after-free. Switch to kref_put_lock() so cfid_release() runs with cfid_list_lock held, closing that gap. While we are at it: * rename the functions smb2_close_cached_fid() and close_cached_dir() for clarity; * replace the calls to kref_put() for cfid_put(). Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com> --- fs/smb/client/cached_dir.c | 28 ++++++++++++++++------------ fs/smb/client/cached_dir.h | 2 +- fs/smb/client/inode.c | 4 ++-- fs/smb/client/readdir.c | 4 ++-- fs/smb/client/smb2inode.c | 2 +- fs/smb/client/smb2ops.c | 8 ++++---- 6 files changed, 26 insertions(+), 22 deletions(-)