Re: [PATCH] Revert "NFSD: Defer sub-object cleanup in export put callbacks"

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2026-05-12 at 20:19 +0800, yangerkun wrote:
> Hi Jeff,
> 
> 在 2026/5/12 19:25, Jeff Layton 写道:
> > On Tue, 2026-05-12 at 10:33 +0800, Yang Erkun wrote:
> > > This reverts commit 48db892356d6cb80f6942885545de4a6dd8d2a29.
> > > 
> > > Commit 48db892356d6 ("NFSD: Defer sub-object cleanup in export put
> > > callbacks") describes an issue where calling svc_export_put, path_put,
> > > and auth_domain_put directly can cause use-after-free (UAF) errors when
> > > accessing ex_path or ex_client->name. However, after discussion in [1],
> > > it is clear that commit e7fcf179b82d ("NFSD: Hold net reference for the
> > > lifetime of /proc/fs/nfs/exports fd") actually resolves this problem.
> > > 
> > > Additionally, commit 48db892356d6 ("NFSD: Defer sub-object cleanup in
> > > export put callbacks") introduces a regression that was already fixed by
> > > commit 69d803c40ede ("nfsd: Revert "nfsd: release svc_expkey/svc_export
> > > with rcu_work""). Therefore, reverting commit 48db892356d6 ("NFSD: Defer
> > > sub-object cleanup in export put callbacks") is necessary to fix this
> > > regression.
> > > 
> > > Link: https://lore.kernel.org/all/10019b42-4589-4f9f-8d5b-d8197db1ce3c@huawei.com/ [1]
> > > Fixes: 48db892356d6 ("NFSD: Defer sub-object cleanup in export put callbacks")
> > > Signed-off-by: Yang Erkun <yangerkun@huawei.com>
> > > ---
> > >   fs/nfsd/export.c | 63 +++++++-----------------------------------------
> > >   fs/nfsd/export.h |  7 ++----
> > >   fs/nfsd/nfsctl.c |  8 +-----
> > >   3 files changed, 12 insertions(+), 66 deletions(-)
> > > 
> > 
> > The LLMs don't seem to agree that this is safe:
> > 
> > commit (not yet applied)
> > Author: Yang Erkun <yangerkun@huawei.com>
> > 
> > Revert "NFSD: Defer sub-object cleanup in export put callbacks"
> > 
> > This reverts commit 48db892356d6. The commit message states that
> > e7fcf179b82d resolves the underlying UAF and that the reverted
> > commit re-introduces an umount regression fixed by 69d803c40ede.
> > 
> > Link: https://lore.kernel.org/all/10019b42-4589-4f9f-8d5b-d8197db1ce3c@huawei.com/
> > 
> > > This reverts commit 48db892356d6cb80f6942885545de4a6dd8d2a29.
> > > 
> > > Commit 48db892356d6 ("NFSD: Defer sub-object cleanup in export put
> > > callbacks") describes an issue where calling svc_export_put, path_put,
> > > and auth_domain_put directly can cause use-after-free (UAF) errors when
> > > accessing ex_path or ex_client->name. However, after discussion in [1],
> > > it is clear that commit e7fcf179b82d ("NFSD: Hold net reference for the
> > > lifetime of /proc/fs/nfs/exports fd") actually resolves this problem.
> > 
> > Is this accurate?  Commit e7fcf179b82d holds a net reference for the
> > lifetime of the /proc/fs/nfs/exports file descriptor.  This prevents
> > nfsd_net_exit() from calling nfsd_export_shutdown() while the exports
> > fd is open, keeping the cache_detail alive.
> 
> Yes.
> 
> > 
> > But the UAF described in 48db892356d6 is a different problem.
> > cache_clean() periodically removes expired entries from the hash table
> > via sunrpc_begin_cache_remove_entry() and drops their reference via
> > cache_put().  If that was the last reference, svc_export_put() runs
> > and frees sub-objects.
> 
> Yes.
> 
> > 
> > Meanwhile, c_show() and e_show() iterate cache entries under
> > rcu_read_lock() without holding a reference.  Specifically, c_show()
> > unconditionally calls cd->cache_show():
> > 
> > net/sunrpc/cache.c:c_show():
> > 	if (cache_check_rcu(cd, cp, NULL))
> > 		seq_puts(m, "# ");
> > 	else if (cache_is_expired(cd, cp))
> > 		seq_puts(m, "# ");
> > 	return cd->cache_show(m, cd, cp);
> > 
> > svc_export_show() then accesses both exp->ex_path via seq_path()
> > and exp->ex_client->name via seq_escape().  expkey_show() similarly
> > accesses ek->ek_client->name and ek->ek_path.
> 
> Yes.
> 
> > 
> > Commit e7fcf179b82d does not prevent cache_clean() from removing
> > individual entries concurrently with these RCU readers.  It seems
> 
> Yes.
> 
> > like the claim in this commit message may be conflating two different
> > problems.
> 
> True, the msg describe here seems confused. That should change to:
> 
> Commit 48db892356d6 ("NFSD: Defer sub-object cleanup in export put
> callbacks") describes an issue where calling svc_export_put, path_put,
> and auth_domain_put directly can cause use-after-free (UAF) errors when
> accessing ex_path or ex_client->name. But after discussion in [1], it 
> seems cannot happen and either will introduce a gression that was
> already fixed by commit 69d803c40ede ("nfsd: Revert "nfsd: release
> svc_expkey/svc_export with rcu_work""). Therefore, reverting commit
> 48db892356d6 ("NFSD: Defer sub-object cleanup in export put callbacks")
> is necessary to fix this regression.
> 
> 
> > 
> > > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> > > index 665153f1720e..0baa58d1dbfc 100644
> > > --- a/fs/nfsd/export.c
> > > +++ b/fs/nfsd/export.c
> > 
> > [ ... ]
> > 
> > > -static void expkey_release(struct work_struct *work)
> > > +static void expkey_put(struct kref *ref)
> > >   {
> > > -	struct svc_expkey *key = container_of(to_rcu_work(work),
> > > -					      struct svc_expkey, ek_rwork);
> > > +	struct svc_expkey *key = container_of(ref, struct svc_expkey, h.ref);
> > >   	if (test_bit(CACHE_VALID, &key->h.flags) &&
> > >   	    !test_bit(CACHE_NEGATIVE, &key->h.flags))
> > >   		path_put(&key->ek_path);
> > >   	auth_domain_put(key->ek_client);
> > > -	kfree(key);
> > > -}
> > > -
> > > -static void expkey_put(struct kref *ref)
> > > -{
> > > -	struct svc_expkey *key = container_of(ref, struct svc_expkey, h.ref);
> > > -
> > > -	INIT_RCU_WORK(&key->ek_rwork, expkey_release);
> > > -	queue_rcu_work(nfsd_export_wq, &key->ek_rwork);
> > > +	kfree_rcu(key, ek_rcu);
> > >   }
> > 
> > With this change, path_put() and auth_domain_put() run immediately
> > when the last reference is dropped, before the RCU grace period.
> > kfree_rcu() only defers the kfree of the svc_expkey struct itself.
> > 
> > If cache_clean() drops the last reference concurrently with an RCU
> > reader in c_show() -> expkey_show(), the reader can access
> > ek->ek_client->name after auth_domain_put() has freed the
> > auth_domain, and can call seq_path(&ek->ek_path) after path_put()
> > has freed the underlying dentry/mnt.  The rcu_read_lock() held by the
> > reader prevents kfree_rcu() from freeing the struct, but does not
> > prevent the direct path_put()/auth_domain_put() calls.
> 
> Yes. It won't prevent the direct path_put/auth_domain_put calls.
> 
> > 
> > Does this re-introduce the UAF that 48db892356d6 was fixing?
> 
> No, something like release of dentry/mnt ek->ek_client->name all 
> protected will call_rcu(call_rcu(&dentry->d_rcu, __d_free) in 
> dentry_free, call_rcu(&mnt->mnt_rcu, delayed_free_vfsmnt) in 
> cleanup_mnt, call_rcu(&dom->rcu_head, svcauth_gss_domain_release_rcu) in 
> svcauth_gss_domain_release, call_rcu(&dom->rcu_head, 
> svcauth_unix_domain_release_rcu) in svcauth_unix_domain_release), so 
> rcu_read_lock/rcu_read_unlock for c_show/e_show can protect this trigger 
> UAF.
> 

Got it. Thanks for the explanation. In that case, I'm fine with
reverting this patch.

Reviewed-by: Jeff Layton <jlayton@kernel.org>