Re: [PATCH v3] ALSA: pcm: Drain proc callbacks at device disconnect

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ALSA: info: snd_info_disconnect_entry() allows proc subtree republishing
--

commit 734aaf0e640d58ff92175c5964010543a8f73ca6
Author: Cen Zhang <zzzccc427@gmail.com>

ALSA: pcm: Drain proc callbacks at device disconnect

This commit introduces snd_info_disconnect_entry() to remove procfs nodes and
drain their callbacks without freeing the info entries, and calls it from
snd_pcm_dev_disconnect() after dropping open_mutex so that buffered prealloc
proc writes cannot run after PCM buffer teardown.

> diff --git a/sound/core/info.c b/sound/core/info.c
> index c4d57d0809adf..023b4736aa6de 100644
> --- a/sound/core/info.c
> +++ b/sound/core/info.c
> @@ -737,6 +737,23 @@ static void snd_info_clear_entries(struct snd_info_entry *entry)
>  	entry->p = NULL;
>  }
>  
> +/**
> + * snd_info_disconnect_entry - remove proc nodes and drain their callbacks
> + * @entry: root of the info entry subtree
> + *
> + * Retains the info entries for snd_info_free_entry().  Callers must serialize
> + * registration and destruction, and must not hold locks needed by callbacks.
> + */
> +void snd_info_disconnect_entry(struct snd_info_entry *entry)
> +{
> +	if (entry && entry->p) {
> +		proc_remove(entry->p);
> +		guard(mutex)(&info_mutex);
> +		snd_info_clear_entries(entry);
> +	}
> +}
> +EXPORT_SYMBOL(snd_info_disconnect_entry);

[Severity: High]
Can a disconnected PCM stream proc subtree be republished if
snd_info_card_register() runs before snd_pcm_free_stream() frees the
entries?

In sound/core/pcm.c, snd_pcm_stream_proc_init() attaches pstr->proc_root to
pcm->card->proc_root->children via snd_info_create_card_entry(). When
snd_pcm_dev_disconnect() calls snd_pcm_stream_proc_disconnect() ->
snd_info_disconnect_entry(pstr->proc_root), snd_info_clear_entries() clears
entry->p to NULL across the subtree while leaving pstr->proc_root linked on
card->proc_root->children:

sound/core/info.c:snd_info_clear_entries() {
	if (!entry->p)
		return;
	list_for_each_entry(p, &entry->children, list)
		snd_info_clear_entries(p);
	entry->p = NULL;
}

During HDA codec unbind in sound/hda/common/bind.c,
hda_codec_driver_remove() disconnects the codec PCMs and then waits for
active references before freeing the PCM devices:

sound/hda/common/bind.c:hda_codec_driver_remove() {
	...
	snd_hda_codec_disconnect_pcms(codec);
	snd_hda_jack_tbl_disconnect(codec);
	snd_refcount_sync(&codec->pcm_ref);
	snd_power_sync_ref(codec->bus->card);

	if (driver->ops->remove)
		driver->ops->remove(codec);
	snd_hda_codec_cleanup_for_unbind(codec);
}

While hda_codec_driver_remove() is waiting in snd_refcount_sync() (for
example, while userspace holds an open PCM file descriptor on that codec),
binding or reconfiguring another codec on the same card calls
hda_codec_driver_probe() (in sound/hda/common/bind.c) or reconfig_codec()
(in sound/hda/common/sysfs.c) -> snd_card_register() ->
snd_info_card_register() -> snd_info_register(card->proc_root):

sound/core/info.c:snd_info_register() {
	if (!entry->p) {
		err = __snd_info_register(entry);
		if (err < 0)
			return err;
	}

	list_for_each_entry(p, &entry->children, list) {
		err = snd_info_register(p);
		if (err < 0)
			return err;
	}
}

Because the disconnected pstr->proc_root subtree is still on
card->proc_root->children with entry->p == NULL, snd_info_register() treats
it as unregistered and calls __snd_info_register() on the subtree,
recreating the procfs nodes (including subN/prealloc).

When the unbinding codec then reaches snd_hda_codec_cleanup_for_unbind() ->
codec_release_pcms() -> snd_device_free(), __snd_device_disconnect() (in
sound/core/device.c) is skipped because dev->state is already
SNDRV_DEV_DISCONNECTED. snd_pcm_free() then calls
snd_pcm_lib_preallocate_free_for_all() while the republished prealloc proc
node is still active, reintroducing the race with
snd_info_text_entry_release() -> snd_pcm_lib_preallocate_proc_write() and
allowing concurrent list traversal on card->proc_root->children between
snd_info_register() and snd_info_free_entry().

Should snd_info_disconnect_entry() unlink the entry from
entry->parent->children or otherwise prevent snd_info_register() from
re-registering a disconnected subtree?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/pm-pmb-2439c851f82c443c3600-v7-2774a482b7f03e1fef60@gmail.com?part=1