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

[Chuck Lever <chuck.lever@oracle.com>]

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