> On Sep 30, 2022, at 3:33 PM, Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
--
Chuck Lever
