Re: [PATCH v3 1/5] ALSA: hda: Serialize codec probe and remove on the same bus

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ALSA: hda: unlocked snd_card_register() races with codec bind and unbind
- [Low] ALSA: hda: guard(mutex) mixed with goto cleanup in hda_codec_driver_probe()
--

commit 98315c01897e919f38855a83f81646c5e30c2565
Author: Cen Zhang <zzzccc427@gmail.com>

ALSA: hda: Serialize codec probe and remove on the same bus

This commit adds codec_bind_lock to struct hdac_bus and acquires it in
hda_codec_driver_probe() and hda_codec_driver_remove() to serialize
snd_card_register() against concurrent codec removal modifying card->devices
on the same HD-audio bus.

> diff --git a/sound/hda/common/bind.c b/sound/hda/common/bind.c
> index 957feace0f58e..abbb3b64ec090 100644
> --- a/sound/hda/common/bind.c
> +++ b/sound/hda/common/bind.c
> @@ -104,6 +104,12 @@ static int hda_codec_driver_probe(struct device *dev)
>  	if (WARN_ON(!codec->preset))
>  		return -EINVAL;
>  
> +	/*
> +	 * snd_card_register() below walks the device list of the card, which
> +	 * the removal of another codec changes when it frees its PCMs.
> +	 */
> +	guard(mutex)(&codec->bus->core.codec_bind_lock);

[Severity: High]
Can card->devices still be corrupted when snd_card_register() is called
outside hda_codec_driver_probe() while another codec on the same bus is
concurrently bound or unbound through sysfs?

In sound/hda/common/sysfs.c, reconfig_codec() calls device_reprobe(), which
acquires and releases codec_bind_lock inside hda_codec_driver_probe(), and
then calls snd_card_register() without holding codec_bind_lock:

sound/hda/common/sysfs.c:reconfig_codec() {
	...
	err = device_reprobe(hda_codec_dev(codec));
	if (err < 0)
		return err;
	return snd_card_register(codec->card);
}

Similarly, the controller probe paths in sound/hda/controllers/intel.c
(azx_probe_continue()), sound/hda/controllers/cix-ipbloq.c
(cix_ipbloq_hda_probe_codec()), sound/hda/controllers/tegra.c
(hda_tegra_probe_work()), and sound/hda/controllers/acpi.c
(hda_acpi_probe_work()) register codec devices via azx_codec_configure()
and then call snd_card_register(chip->card) without holding codec_bind_lock
(and in tegra.c and acpi.c without setting bus_probing).

If another codec on the same bus is unbound or bound via sysfs at the same
time, hda_codec_driver_remove() -> snd_hda_codec_cleanup_for_unbind() ->
codec_release_pcms() -> snd_device_free() or hda_codec_driver_probe() ->
snd_hda_codec_build_pcms() -> snd_pcm_new() -> snd_device_new() will mutate
card->devices and free struct snd_device entries while the unlocked
snd_card_register() -> snd_device_register_all() walks card->devices.

> +
>  	err = snd_hda_codec_set_name(codec, codec->preset->name);
>  	if (err < 0)
>  		goto error;

[Severity: Low]
This isn't a bug since guard(mutex) is declared before any jump, but
hda_codec_driver_probe() in sound/hda/common/bind.c still uses goto error,
goto error_module_put, and goto error_module for cleanup:

sound/hda/common/bind.c:hda_codec_driver_probe() {
	...
 error_module:
	snd_hdac_device_disable_unsol(&codec->core);
	cancel_delayed_work_sync(&codec->jackpoll_work);
	if (probed && driver->ops->remove)
		driver->ops->remove(codec);
 error_module_put:
	module_put(owner);

 error:
	snd_hda_codec_cleanup_for_unbind(codec);
	codec->preset = NULL;
	return err;
}

Per the cleanup.h guidelines on not mixing guard() with goto error handling
in the same function, could the probe body be factored into a helper called
under guard(mutex), or use explicit mutex_lock() and mutex_unlock() instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/pm-pmb-a4703edca61d34aa679b-v11-d9186d19e8afb48972af-0@gmail.com?part=1