On Sat, 2022-10-01 at 11:48 -0400, Chuck Lever wrote:
> nfsd_file is RCU-freed, so it's possible that one could be found
> that's in the process of being freed and the memory recycled. Ensure
> we hold the rcu_read_lock while attempting to get a reference on the
> object.
>
> Suggested-by: Jeff Layton <jlayton@xxxxxxxxxx>
> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> ---
> fs/nfsd/filecache.c | 34 +++++++++++-----------------------
> fs/nfsd/trace.h | 27 ---------------------------
> 2 files changed, 11 insertions(+), 50 deletions(-)
>
> This is what I was thinking... Compile-tested only.
>
>
Looks reasonable. I had something pretty similar that I'll send along in
a bit.
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index be152e3e3a80..6e17f74fb29f 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -1056,10 +1056,12 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
>
> retry:
> /* Avoid allocation if the item is already in cache */
> - nf = rhashtable_lookup_fast(&nfsd_file_rhash_tbl, &key,
> - nfsd_file_rhash_params);
> + rcu_read_lock();
> + nf = rhashtable_lookup(&nfsd_file_rhash_tbl, &key,
> + nfsd_file_rhash_params);
> if (nf)
> nf = nfsd_file_get(nf);
> + rcu_read_unlock();
> if (nf)
> goto wait_for_construction;
>
> @@ -1069,21 +1071,14 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> goto out_status;
> }
>
> - nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl,
> - &key, &new->nf_rhash,
> - nfsd_file_rhash_params);
> - if (!nf) {
> - nf = new;
> - goto open_file;
> - }
> - if (IS_ERR(nf))
> - goto insert_err;
> - nf = nfsd_file_get(nf);
> - if (nf == NULL) {
> - nf = new;
> - goto open_file;
> + if (rhashtable_lookup_insert_key(&nfsd_file_rhash_tbl,
> + &key, &new->nf_rhash,
> + nfsd_file_rhash_params)) {
> + nfsd_file_slab_free(&new->nf_rcu);
> + goto retry;
This can return other errors besides -EEXIST. I'm not sure we want to
goto retry on those others.
> }
> - nfsd_file_slab_free(&new->nf_rcu);
> + nf = new;
> + goto open_file;
>
> wait_for_construction:
> wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE);
> @@ -1143,13 +1138,6 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> smp_mb__after_atomic();
> wake_up_bit(&nf->nf_flags, NFSD_FILE_PENDING);
> goto out;
> -
> -insert_err:
> - nfsd_file_slab_free(&new->nf_rcu);
> - trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, PTR_ERR(nf));
> - nf = NULL;
> - status = nfserr_jukebox;
> - goto out_status;
> }
>
> /**
> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> index 06a96e955bd0..c15467b2e8d9 100644
> --- a/fs/nfsd/trace.h
> +++ b/fs/nfsd/trace.h
> @@ -954,33 +954,6 @@ TRACE_EVENT(nfsd_file_create,
> )
> );
>
> -TRACE_EVENT(nfsd_file_insert_err,
> - TP_PROTO(
> - const struct svc_rqst *rqstp,
> - const struct inode *inode,
> - unsigned int may_flags,
> - long error
> - ),
> - TP_ARGS(rqstp, inode, may_flags, error),
> - TP_STRUCT__entry(
> - __field(u32, xid)
> - __field(const void *, inode)
> - __field(unsigned long, may_flags)
> - __field(long, error)
> - ),
> - TP_fast_assign(
> - __entry->xid = be32_to_cpu(rqstp->rq_xid);
> - __entry->inode = inode;
> - __entry->may_flags = may_flags;
> - __entry->error = error;
> - ),
> - TP_printk("xid=0x%x inode=%p may_flags=%s error=%ld",
> - __entry->xid, __entry->inode,
> - show_nfsd_may_flags(__entry->may_flags),
> - __entry->error
> - )
> -);
> -
> TRACE_EVENT(nfsd_file_cons_err,
> TP_PROTO(
> const struct svc_rqst *rqstp,
>
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
