| Message ID | 20220930191550.172087-2-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: filecache fixes | expand |
> On Sep 30, 2022, at 3:15 PM, Jeff Layton <jlayton@kernel.org> 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. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> IIUC, the rcu_read_lock() is held when nfsd_file_obj_cmpfn() is invoked. So, couldn't we just call nfsd_file_get() on @nf in there if it returns a match? > --- > fs/nfsd/filecache.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index d5c57360b418..6237715bd23e 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -1077,10 +1077,12 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > > retry: > /* Avoid allocation if the item is already in cache */ > + rcu_read_lock(); > nf = rhashtable_lookup_fast(&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; > > @@ -1090,16 +1092,21 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > goto out_status; > } > > + rcu_read_lock(); > nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl, > &key, &new->nf_rhash, > nfsd_file_rhash_params); > if (!nf) { > + rcu_read_unlock(); > nf = new; > goto open_file; > } > - if (IS_ERR(nf)) > + if (IS_ERR(nf)) { > + rcu_read_unlock(); > goto insert_err; > + } > nf = nfsd_file_get(nf); > + rcu_read_unlock(); > if (nf == NULL) { > nf = new; > goto open_file; > -- > 2.37.3 > -- Chuck Lever
On Fri, 2022-09-30 at 19:20 +0000, Chuck Lever III wrote: > > > On Sep 30, 2022, at 3:15 PM, Jeff Layton <jlayton@kernel.org> 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. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > IIUC, the rcu_read_lock() is held when nfsd_file_obj_cmpfn() is > invoked. So, couldn't we just call nfsd_file_get() on @nf in > there if it returns a match? > > Adding side effects to the comparison function seems kind of gross. Do you know for sure that it will only be called once when you do a search? Also, there are other places where we search the hashtable but don't take references. We could just make those places put the reference, but that gets messy. I think this is cleaner. > > --- > > fs/nfsd/filecache.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > > index d5c57360b418..6237715bd23e 100644 > > --- a/fs/nfsd/filecache.c > > +++ b/fs/nfsd/filecache.c > > @@ -1077,10 +1077,12 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > > > > retry: > > /* Avoid allocation if the item is already in cache */ > > + rcu_read_lock(); > > nf = rhashtable_lookup_fast(&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; > > > > @@ -1090,16 +1092,21 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > > goto out_status; > > } > > > > + rcu_read_lock(); > > nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl, > > &key, &new->nf_rhash, > > nfsd_file_rhash_params); > > if (!nf) { > > + rcu_read_unlock(); > > nf = new; > > goto open_file; > > } > > - if (IS_ERR(nf)) > > + if (IS_ERR(nf)) { > > + rcu_read_unlock(); > > goto insert_err; > > + } > > nf = nfsd_file_get(nf); > > + rcu_read_unlock(); > > if (nf == NULL) { > > nf = new; > > goto open_file; > > -- > > 2.37.3 > > > > -- > Chuck Lever > > >
> On Sep 30, 2022, at 3:33 PM, Jeff Layton <jlayton@kernel.org> wrote: > > On Fri, 2022-09-30 at 19:20 +0000, Chuck Lever III wrote: >> >>> On Sep 30, 2022, at 3:15 PM, Jeff Layton <jlayton@kernel.org> 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. >>> >>> Signed-off-by: Jeff Layton <jlayton@kernel.org> >> >> IIUC, the rcu_read_lock() is held when nfsd_file_obj_cmpfn() is >> invoked. So, couldn't we just call nfsd_file_get() on @nf in >> there if it returns a match? >> >> > > Adding side effects to the comparison function seems kind of gross. > Do you know for sure that it will only be called once when you do a search? Yes, when a match occurs, the chain walk stops. > Also, there are other places where we search the hashtable but don't > take references. We could just make those places put the reference, but > that gets messy. nfsd_file_do_acquire() is the only place that calls the comparator with NFSD_FILE_KEY_FULL. 165 if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags) == 0) 166 return 1; + nfsd_file_get(nf); 167 break; > I think this is cleaner. Unfortunately, the comparator's second parameter is a const void *, so we can't modify the matching nf. Ho-hum. >>> --- >>> fs/nfsd/filecache.c | 9 ++++++++- >>> 1 file changed, 8 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c >>> index d5c57360b418..6237715bd23e 100644 >>> --- a/fs/nfsd/filecache.c >>> +++ b/fs/nfsd/filecache.c >>> @@ -1077,10 +1077,12 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, >>> >>> retry: >>> /* Avoid allocation if the item is already in cache */ >>> + rcu_read_lock(); >>> nf = rhashtable_lookup_fast(&nfsd_file_rhash_tbl, &key, >>> nfsd_file_rhash_params); This probably should be changed to rhashtable_lookup() so that we're not double-wrapping the call with rcu_read_lock/unlock. >>> if (nf) >>> nf = nfsd_file_get(nf); >>> + rcu_read_unlock(); >>> if (nf) >>> goto wait_for_construction; >>> >>> @@ -1090,16 +1092,21 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, >>> goto out_status; >>> } >>> >>> + rcu_read_lock(); >>> nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl, >>> &key, &new->nf_rhash, >>> nfsd_file_rhash_params); >>> if (!nf) { >>> + rcu_read_unlock(); >>> nf = new; >>> goto open_file; >>> } >>> - if (IS_ERR(nf)) >>> + if (IS_ERR(nf)) { >>> + rcu_read_unlock(); >>> goto insert_err; >>> + } >>> nf = nfsd_file_get(nf); >>> + rcu_read_unlock(); What might be nicer is if this used rhashtable_lookup_insert_key() instead, and if it returns EEXIST, it simply frees @new and retries the first lookup. >>> if (nf == NULL) { >>> nf = new; >>> goto open_file; >>> -- >>> 2.37.3 >>> >> >> -- >> Chuck Lever >> >> >> > > -- > Jeff Layton <jlayton@kernel.org> -- Chuck Lever
On Sat, 01 Oct 2022, Jeff Layton 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. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/filecache.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index d5c57360b418..6237715bd23e 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -1077,10 +1077,12 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > > retry: > /* Avoid allocation if the item is already in cache */ > + rcu_read_lock(); > nf = rhashtable_lookup_fast(&nfsd_file_rhash_tbl, &key, > nfsd_file_rhash_params); > if (nf) > nf = nfsd_file_get(nf); > + rcu_read_unlock(); Looks good. > if (nf) > goto wait_for_construction; > > @@ -1090,16 +1092,21 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > goto out_status; > } > > + rcu_read_lock(); > nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl, > &key, &new->nf_rhash, > nfsd_file_rhash_params); > if (!nf) { > + rcu_read_unlock(); > nf = new; > goto open_file; > } > - if (IS_ERR(nf)) > + if (IS_ERR(nf)) { > + rcu_read_unlock(); > goto insert_err; > + } > nf = nfsd_file_get(nf); > + rcu_read_unlock(); Ugh. Could we make this: rcu_read_lock() nf = r_l_g_i_k() if (!IS_ERR_OR_NULL(nf)) nf = nfsd_file_get(nf); rcu_read_unlock() ... ?? NeilBrown > if (nf == NULL) { > nf = new; > goto open_file; > -- > 2.37.3 > >
On Sat, 2022-10-01 at 14:44 +1000, NeilBrown wrote: > On Sat, 01 Oct 2022, Jeff Layton 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. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/nfsd/filecache.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > > index d5c57360b418..6237715bd23e 100644 > > --- a/fs/nfsd/filecache.c > > +++ b/fs/nfsd/filecache.c > > @@ -1077,10 +1077,12 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > > > > retry: > > /* Avoid allocation if the item is already in cache */ > > + rcu_read_lock(); > > nf = rhashtable_lookup_fast(&nfsd_file_rhash_tbl, &key, > > nfsd_file_rhash_params); > > if (nf) > > nf = nfsd_file_get(nf); > > + rcu_read_unlock(); > > Looks good. > > > if (nf) > > goto wait_for_construction; > > > > @@ -1090,16 +1092,21 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > > goto out_status; > > } > > > > + rcu_read_lock(); > > nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl, > > &key, &new->nf_rhash, > > nfsd_file_rhash_params); > > if (!nf) { > > + rcu_read_unlock(); > > nf = new; > > goto open_file; > > } > > - if (IS_ERR(nf)) > > + if (IS_ERR(nf)) { > > + rcu_read_unlock(); > > goto insert_err; > > + } > > nf = nfsd_file_get(nf); > > + rcu_read_unlock(); > > Ugh. > Could we make this: > rcu_read_lock() > nf = r_l_g_i_k() > if (!IS_ERR_OR_NULL(nf)) > nf = nfsd_file_get(nf); > rcu_read_unlock() > ... > ?? > > NeilBrown > Good point. I'll resend a v2 here. > > if (nf == NULL) { > > nf = new; > > goto open_file; > > -- > > 2.37.3 > > > >
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c index d5c57360b418..6237715bd23e 100644 --- a/fs/nfsd/filecache.c +++ b/fs/nfsd/filecache.c @@ -1077,10 +1077,12 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, retry: /* Avoid allocation if the item is already in cache */ + rcu_read_lock(); nf = rhashtable_lookup_fast(&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; @@ -1090,16 +1092,21 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, goto out_status; } + rcu_read_lock(); nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl, &key, &new->nf_rhash, nfsd_file_rhash_params); if (!nf) { + rcu_read_unlock(); nf = new; goto open_file; } - if (IS_ERR(nf)) + if (IS_ERR(nf)) { + rcu_read_unlock(); goto insert_err; + } nf = nfsd_file_get(nf); + rcu_read_unlock(); if (nf == NULL) { nf = new; goto open_file;
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. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/filecache.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)