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 <[email protected]>
Signed-off-by: Chuck Lever <[email protected]>
---
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();
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,
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 <[email protected]>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> 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
[email protected]