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

[Jeff Layton <jlayton@kernel.org>]

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?
-- 
Jeff Layton <jlayton@kernel.org>