Re: [RFC PATCH v3 04/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 2 potential issue(s) to consider:
- [High] smb: client: WQ_MEM_RECLAIM flush dependency violation in sesInfoFree()
- [Medium] smb: client: WARN_ON() on fallible keyring_clear() in cache clear helpers
--

commit 359186c48dde297d60c971ef75e736ee0f7821b6
Author: Jarkko Sakkinen <jarkko@kernel.org>

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

This commit serializes SMB session count transitions with a mutex and clears
the cifs.idmap and cifs.spnego cache keyrings when the last SMB session is
freed so that cached keys do not keep the module pinned.

> diff --git a/fs/smb/client/cifs_spnego.c b/fs/smb/client/cifs_spnego.c
> index 44c4072756804..939e14e755e52 100644
> --- a/fs/smb/client/cifs_spnego.c
> +++ b/fs/smb/client/cifs_spnego.c
> @@ -190,6 +190,12 @@ cifs_get_spnego_key(struct cifs_ses *sesInfo,
>  	return spnego_key;
>  }
>  
> +void
> +cifs_spnego_clear_cache(void)
> +{
> +	WARN_ON(keyring_clear(spnego_cred->thread_keyring));
> +}
> +
>  int
>  init_cifs_spnego(void)
>  {
> diff --git a/fs/smb/client/cifsacl.c b/fs/smb/client/cifsacl.c
> index 346899da91eb2..126b976c2547f 100644
> --- a/fs/smb/client/cifsacl.c
> +++ b/fs/smb/client/cifsacl.c
> @@ -539,6 +539,12 @@ sid_to_id(struct cifs_sb_info *cifs_sb, struct smb_sid *psid,
>  	return 0;
>  }
>  
> +void
> +cifs_idmap_clear_cache(void)
> +{
> +	WARN_ON(keyring_clear(root_cred->thread_keyring));
> +}

[Severity: Medium]
As reported by Sashiko on RFC v2 [1], can cifs_spnego_clear_cache() and
cifs_idmap_clear_cache() trigger a WARN_ON() splat (and crash on systems with
panic_on_warn enabled) when memory allocation fails during session teardown?

When sesInfoFree() frees the last session, it calls cifs_idmap_clear_cache()
and cifs_spnego_clear_cache(), which pass the return value of keyring_clear()
directly to WARN_ON().

In security/keys/keyring.c:keyring_clear():

	edit = assoc_array_clear(&keyring->keys, &keyring_assoc_array_ops);
	if (IS_ERR(edit)) {
		ret = PTR_ERR(edit);

And in lib/assoc_array.c:assoc_array_clear(), clearing a non-empty associative
array performs a GFP_KERNEL allocation:

	edit = kzalloc_obj(struct assoc_array_edit);
	if (!edit)
		return ERR_PTR(-ENOMEM);

If this allocation fails under memory pressure, memcg limits, or fault
injection while either keyring contains cached keys, keyring_clear() returns
-ENOMEM and triggers WARN_ON().

Notably, the companion NFSv4 patch in the same series checks the return value
of keyring_clear(), logs with pr_warn_ratelimited(), and skips key_gc_flush()
on failure. Should cifs_idmap_clear_cache() and cifs_spnego_clear_cache()
handle keyring_clear() 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..11c0330be489b 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(&sesInfoAllocCount)) {
> +		cifs_idmap_clear_cache();
> +		cifs_spnego_clear_cache();
> +		key_gc_flush();
> +	}
> +	mutex_unlock(&cifs_session_mutex);
>  }

[Severity: High]
As reported by Sashiko on RFC v2 [2], does calling key_gc_flush() from
sesInfoFree() cause a WQ_MEM_RECLAIM workqueue flush dependency violation when
the final session reference is dropped from a cifsiod_wq worker?

When keyring_clear() unlinks cached keys, it schedules assoc_array_rcu_cleanup
via call_rcu(). Inside security/keys/gc.c:key_gc_flush():

void key_gc_flush(void)
{
	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 the default system workqueue (which
does not have WQ_MEM_RECLAIM set), and key_gc_flush() then calls
flush_work(&key_gc_work).

In fs/smb/client/cifsfs.c:init_cifs(), cifsiod_wq is allocated with
WQ_MEM_RECLAIM:

	cifsiod_wq = alloc_workqueue("cifsiod",
				     WQ_FREEZABLE | WQ_MEM_RECLAIM | WQ_PERCPU,
				     0);

Because cifs_kill_sb() does not flush cifsiod_wq before calling cifs_umount(),
background work items running on cifsiod_wq can outlive unmount and drop the
last reference to struct cifs_ses, including:

  mchan_mount_work_fn() -> mchan_mount_free() -> cifs_put_smb_ses()
  smb2_reconnect_server() -> cifs_put_tcon() / cifs_put_smb_ses()
  smb2_cancelled_close_fid() -> cifs_put_tcon()
  cifs_ses_oplock_break() -> cifs_put_smb_ses()

When any of those workers drops the final reference, __cifs_put_smb_ses() ->
sesInfoFree() -> key_gc_flush() -> flush_work(&key_gc_work) flushes a
!WQ_MEM_RECLAIM workqueue from a WQ_MEM_RECLAIM worker, triggering WARN_ONCE()
in kernel/workqueue.c:check_flush_dependency() and risking a memory-reclaim
deadlock.

[1] https://lore.kernel.org/r/20261103030226.2685251-6-jarkko@kernel.org
[2] https://lore.kernel.org/r/20261103030226.2685251-6-jarkko@kernel.org

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