Re: [PATCH RFC] NFSD: Hold rcu_read_lock while getting refs

[Jeff Layton <jlayton@kernel.org>]

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@kernel.org>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  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@kernel.org>