Re: [PATCH] nfsd: free nfsd_file by gc after adding it to lru list

Chuck Lever
2025-01-15T15:04:35Z

On 1/14/25 2:39 PM, Jeff Layton wrote:
> On Tue, 2025-01-14 at 14:27 -0500, Jeff Layton wrote:
>> On Mon, 2025-01-13 at 10:59 +0800, Li Lingfeng wrote:
>>> In nfsd_file_put, after inserting the nfsd_file into the nfsd_file_lru
>>> list, gc may be triggered in another thread and immediately release this
>>> nfsd_file, which will lead to a UAF when accessing this nfsd_file again.
>>>
>>> All the places where unhash is done will also perform lru_remove, so there
>>> is no need to do lru_remove separately here. After inserting the nfsd_file
>>> into the nfsd_file_lru list, it can be released by relying on gc.
>>>
>>> Fixes: 4a0e73e635e3 ("NFSD: Leave open files out of the filecache LRU")
>>> Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
>>> ---
>>> fs/nfsd/filecache.c | 12 ++----------
>>> 1 file changed, 2 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>>> index a1cdba42c4fa..37b65cb1579a 100644
>>> --- a/fs/nfsd/filecache.c
>>> +++ b/fs/nfsd/filecache.c
>>> @@ -372,18 +372,10 @@ nfsd_file_put(struct nfsd_file *nf)
>>> /* Try to add it to the LRU. If that fails, decrement. */
>>> if (nfsd_file_lru_add(nf)) {
>>> /* If it's still hashed, we're done */
>>> - if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) {
>>> + if (list_lru_count(&nfsd_file_lru))
>>> nfsd_file_schedule_laundrette();
>>> - return;
>>> - }
>>>
>>> - /*
>>> - * We're racing with unhashing, so try to remove it from
>>> - * the LRU. If removal fails, then someone else already
>>> - * has our reference.
>>> - */
>>> - if (!nfsd_file_lru_remove(nf))
>>> - return;
>>> + return;
>>> }
>>> }
>>> if (refcount_dec_and_test(&nf->nf_ref))
>>
>> I think this looks OK. Filecache bugs are particularly nasty though, so
>> let's run this through a nice long testing cycle.
>>
>> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>
> Actually, I take it back. This is problematic in another way.
>
> In this case, we're racing with another task that is unhashing the
> object, but we've put it on the LRU ourselves. What guarantee do we
> have that the unhashing and removal from the LRU didn't occur before
> this task called nfsd_file_lru_add()? That's why we attempt to remove
> it here -- we can't rely on the task that unhashed it to do so at that
> point.
>
> What might be best is to take and hold the rcu_read_lock() before doing
> the nfsd_file_lru_add, and just release it after we do these racy
> checks. That should make it safe to access the object.
>
> Thoughts?

Holding the RCU read lock will keep the dereferences safe since
nfsd_file objects are freed only after an RCU grace period. But will the
logic in nfsd_file_put() work properly on totally dead nfsd_file
objects? I don't see a specific failure mode there, but I'm not very
imaginative.

Overall, I think RCU would help.

--
Chuck Lever