Re: [RFC PATCH v2 05/11] smb: client: Clear idmap/SPNEGO caches when the last session is unloaded

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] smb: client: key_gc_flush() in sesInfoFree() on WQ_MEM_RECLAIM workqueues
- [Medium] smb: client: WARN_ON() on keyring_clear() failure in cache clear helpers
- [Medium] smb: client: premature SPNEGO cache clear in dfs_mount_share()
--

commit 1af6c60c9338c6ee58b722558872d2fff3e07106
Author: Jarkko Sakkinen <jarkko@kernel.org>

smb: client: Clear idmap/SPNEGO caches when the last session is unloaded

This commit serializes CIFS session count transitions with a mutex and clears
the idmap and SPNEGO cache keyrings, flushing key garbage collection when the
last SMB session is freed.

> diff --git a/fs/smb/client/cifs_spnego.c b/fs/smb/client/cifs_spnego.c
> index 44c4072756804..3bd272c7d9fe6 100644
> --- a/fs/smb/client/cifs_spnego.c
> +++ b/fs/smb/client/cifs_spnego.c
> @@ -242,6 +242,14 @@ init_cifs_spnego(void)
>  	return ret;
>  }
>  
> +void cifs_spnego_clear_cache(void)
> +{
> +	int ret;
> +
> +	ret = keyring_clear(spnego_cred->thread_keyring);
> +	WARN_ON(ret);
> +}
> +
>  void
>  exit_cifs_spnego(void)
>  {
> diff --git a/fs/smb/client/cifsacl.c b/fs/smb/client/cifsacl.c
> index 346899da91eb2..7504cedf64177 100644
> --- a/fs/smb/client/cifsacl.c
> +++ b/fs/smb/client/cifsacl.c
> @@ -590,6 +590,14 @@ init_cifs_idmap(void)
>  	return ret;
>  }
>  
> +void cifs_idmap_clear_cache(void)
> +{
> +	int ret;
> +
> +	ret = keyring_clear(root_cred->thread_keyring);
> +	WARN_ON(ret);
> +}

[Severity: Medium]
Can keyring_clear() in cifs_spnego_clear_cache() and cifs_idmap_clear_cache()
trigger a WARN_ON() on a normal memory allocation failure?

When sesInfoFree() frees the last SMB session while keys are cached in
spnego_cred->thread_keyring or root_cred->thread_keyring, keyring_clear()
calls assoc_array_clear():

security/keys/keyring.c:keyring_clear()
  assoc_array_clear(&keyring->keys, &keyring_assoc_array_ops)
    kzalloc_obj(struct assoc_array_edit)

If that GFP_KERNEL allocation fails under memory pressure, memcg limits, or
fault injection, assoc_array_clear() returns ERR_PTR(-ENOMEM) and
keyring_clear() returns -ENOMEM, causing WARN_ON(ret) to fire (and panicking
systems with panic_on_warn enabled).

Other commits in this series (such as nfs_idmap_delete() in
fs/nfs/nfs4idmap.c and dns_resolver_cache_put() in net/dns_resolver/dns_key.c)
log a ratelimited warning with pr_warn_ratelimited() and skip key_gc_flush()
when keyring_clear() fails. Should cifs_spnego_clear_cache() and
cifs_idmap_clear_cache() handle errors the same way instead of using
WARN_ON()?

> diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
> index ec28f102ea08a..4503434942f97 100644
> --- a/fs/smb/client/misc.c
> +++ b/fs/smb/client/misc.c

[ ... ]

> @@ -115,6 +122,14 @@ sesInfoFree(struct cifs_ses *buf_to_free)
>  		kref_put(&iface->refcount, release_iface);
>  	spin_unlock(&buf_to_free->iface_lock);
>  	kfree_sensitive(buf_to_free);
> +
> +	mutex_lock(&cifs_session_mutex);
> +	if (atomic_dec_and_test(&cifs_ses_alloc_count)) {
> +		cifs_idmap_clear_cache();
> +		cifs_spnego_clear_cache();

[Severity: Medium]
When mounting a DFS share with no existing SMB sessions active, does clearing
the caches whenever cifs_ses_alloc_count drops to zero prematurely evict the
SPNEGO key just obtained during the mount?

In fs/smb/client/dfs.c:dfs_mount_share(), when !ctx->dfs_conn initially, the
mount path sets up a session (populating spnego_cred->thread_keyring via an
upcall), queries the DFS referral, and then tears down that initial connection
before allocating the new DFS session:

dfs_mount_share()
  get_session(mnt_ctx, NULL)        /* cifs_ses_alloc_count: 0 -> 1 */
  dfs_get_referral(...)
  if (!ctx->dfs_conn) {
      ctx->dfs_conn = true;
      cifs_mount_put_conns(mnt_ctx) /* cifs_ses_alloc_count: 1 -> 0 */
      rc = get_session(mnt_ctx, NULL);
  }

Because cifs_mount_put_conns() calls cifs_put_smb_ses() ->
__cifs_put_smb_ses() -> sesInfoFree() before the replacement session is
allocated, cifs_ses_alloc_count transiently reaches 0. This wipes the SPNEGO
keyring and stalls in key_gc_flush(), forcing the immediately following
get_session() call to perform a redundant userspace SPNEGO upcall.

> +		key_gc_flush();
> +	}
> +	mutex_unlock(&cifs_session_mutex);
>  }

[Severity: High]
What happens if the last reference to struct cifs_ses is dropped from a
workqueue created with WQ_MEM_RECLAIM, such as cifsiod_wq?

In init_cifs(), cifsiod_wq is allocated with WQ_MEM_RECLAIM, and
cifs_kill_sb() does not flush cifsiod_wq before calling cifs_umount().
Background work items running on cifsiod_wq can hold session or tcon
references and drop the final session reference if unmount completes
concurrently:

fs/smb/client/connect.c:mchan_mount_work_fn() [on cifsiod_wq]
  mchan_mount_free()
    cifs_put_smb_ses()
      __cifs_put_smb_ses()
        sesInfoFree()

or:

fs/smb/client/smb2pdu.c:smb2_reconnect_server() [on cifsiod_wq]
  cifs_put_smb_ses() / cifs_put_tcon()
    __cifs_put_smb_ses()
      sesInfoFree()

When sesInfoFree() clears non-empty keyrings and calls key_gc_flush():

security/keys/gc.c:key_gc_flush() {
	do {
		rcu_barrier();
	} while (flush_work(&key_gc_work));
}

rcu_barrier() runs assoc_array_rcu_cleanup() -> keyring_free_object() ->
key_put(), which schedules key_gc_work on system_dfl_wq (a workqueue without
WQ_MEM_RECLAIM). Calling flush_work(&key_gc_work) from a WQ_MEM_RECLAIM worker
on cifsiod_wq will then trigger the WARN_ONCE() in check_flush_dependency()
for flushing a !WQ_MEM_RECLAIM workqueue from a memory-reclaim worker.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103030226.2685251-1-jarkko@kernel.org?part=5