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

[yangerkun <yangerkun@huawei.com>]

在 2026/5/12 20:30, Jeff Layton 写道:
> 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>

Thanks a lot for your review!

>