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

[Trond Myklebust <trondmy@hammerspace.com>]

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.
> 
> 
> 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();
> 

That definitely deserves a 'Fixes:' line so you can unbreak 6.0.

>         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;
>         }
> -       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,
> 
> 

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com