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

[Jeff Layton <jlayton@kernel.org>]

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.

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.

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.

Commit e7fcf179b82d does not prevent cache_clean() from removing
individual entries concurrently with these RCU readers.  It seems
like the claim in this commit message may be conflating two different
problems.

> 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.

Does this re-introduce the UAF that 48db892356d6 was fixing?

> -static void svc_export_release(struct work_struct *work)
> +static void svc_export_release(struct rcu_head *rcu_head)
>  {
> -	struct svc_export *exp = container_of(to_rcu_work(work),
> -					      struct svc_export, ex_rwork);
> +	struct svc_export *exp = container_of(rcu_head, struct svc_export,
> +			ex_rcu);
>
> -	path_put(&exp->ex_path);
> -	auth_domain_put(exp->ex_client);
>  	nfsd4_fslocs_free(&exp->ex_fslocs);
>  	export_stats_destroy(exp->ex_stats);
>  	kfree(exp->ex_stats);
> @@ -382,8 +369,9 @@ static void svc_export_put(struct kref *ref)
>  {
>  	struct svc_export *exp = container_of(ref, struct svc_export, h.ref);
>
> -	INIT_RCU_WORK(&exp->ex_rwork, svc_export_release);
> -	queue_rcu_work(nfsd_export_wq, &exp->ex_rwork);
> +	path_put(&exp->ex_path);
> +	auth_domain_put(exp->ex_client);
> +	call_rcu(&exp->ex_rcu, svc_export_release);
>  }

Same concern here.  With path_put() and auth_domain_put() called
directly in svc_export_put() before call_rcu(), a concurrent RCU
reader in e_show() -> svc_export_show() or c_show() ->
svc_export_show() could dereference freed sub-objects through
seq_path(&exp->ex_path) or exp->ex_client->name.

-- 
Jeff Layton <jlayton@kernel.org>