* [PATCH RFC] NFSD: Hold rcu_read_lock while getting refs
@ 2022-10-01 15:48 Chuck Lever
2022-10-03 11:34 ` Jeff Layton
2022-10-03 13:16 ` Trond Myklebust
0 siblings, 2 replies; 3+ messages in thread
From: Chuck Lever @ 2022-10-01 15:48 UTC (permalink / raw)
To: linux-nfs
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();
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,
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH RFC] NFSD: Hold rcu_read_lock while getting refs
2022-10-01 15:48 [PATCH RFC] NFSD: Hold rcu_read_lock while getting refs Chuck Lever
@ 2022-10-03 11:34 ` Jeff Layton
2022-10-03 13:16 ` Trond Myklebust
1 sibling, 0 replies; 3+ messages in thread
From: Jeff Layton @ 2022-10-03 11:34 UTC (permalink / raw)
To: Chuck Lever, linux-nfs
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>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH RFC] NFSD: Hold rcu_read_lock while getting refs
2022-10-01 15:48 [PATCH RFC] NFSD: Hold rcu_read_lock while getting refs Chuck Lever
2022-10-03 11:34 ` Jeff Layton
@ 2022-10-03 13:16 ` Trond Myklebust
1 sibling, 0 replies; 3+ messages in thread
From: Trond Myklebust @ 2022-10-03 13:16 UTC (permalink / raw)
To: linux-nfs@vger.kernel.org, chuck.lever@oracle.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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-10-03 13:17 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-10-01 15:48 [PATCH RFC] NFSD: Hold rcu_read_lock while getting refs Chuck Lever
2022-10-03 11:34 ` Jeff Layton
2022-10-03 13:16 ` Trond Myklebust
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).