[PATCH] ALSA: hda: Do not unset preset when cleaning up codec
Several functions that take part in codec's initialization and removal are re-used by ASoC codec drivers implementations. Drivers mimic the behavior of hda_codec_driver_probe/remove() found in sound/pci/hda/hda_bind.c with their component->probe/remove() instead.
One of the reasons for that is the expectation of snd_hda_codec_device_new() to receive a valid struct snd_card pointer what cannot be fulfilled on ASoC side until a card is attempted to be bound and its component probing is triggered.
As ASoC sound card may be unbound without codec device being actually removed from the system, unsetting ->preset in snd_hda_codec_cleanup_for_unbind() interferes with module unload -> load scenario causing null-ptr-deref. Preset is assigned only once, during device/driver matching whereas ASoC codec driver's module reloading may occur several times throughout the lifetime of an audio stack.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com ---
This is a continuation of a discussion that begun in the middle of 2022 [1] and was part of a larger series addressing several HDAudio topics.
Single rmmod on ASoC's codec driver module is enough to cause a panic. Given our results, no regression shows up with modprobe/rmmod on snd_hda_intel side with this patch applied.
[1]: https://lore.kernel.org/alsa-devel/20220706120230.427296-2-cezary.rojewski@i...
sound/pci/hda/hda_codec.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index edd653ece70d..ac1cc7c5290e 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -795,7 +795,6 @@ void snd_hda_codec_cleanup_for_unbind(struct hda_codec *codec) snd_array_free(&codec->cvt_setups); snd_array_free(&codec->spdif_out); snd_array_free(&codec->verbs); - codec->preset = NULL; codec->follower_dig_outs = NULL; codec->spdif_status_reset = 0; snd_array_free(&codec->mixers);
On 1/17/23 09:47, Cezary Rojewski wrote:
Several functions that take part in codec's initialization and removal are re-used by ASoC codec drivers implementations. Drivers mimic the behavior of hda_codec_driver_probe/remove() found in sound/pci/hda/hda_bind.c with their component->probe/remove() instead.
One of the reasons for that is the expectation of snd_hda_codec_device_new() to receive a valid struct snd_card pointer what cannot be fulfilled on ASoC side until a card is attempted to be
very hard to follow. Is there a spurious 'what' to be removed? Or is there missing text? Please consider rewording with simpler sentences.
bound and its component probing is triggered.
As ASoC sound card may be unbound without codec device being actually removed from the system, unsetting ->preset in snd_hda_codec_cleanup_for_unbind() interferes with module unload -> load scenario causing null-ptr-deref. Preset is assigned only once, during device/driver matching whereas ASoC codec driver's module reloading may occur several times throughout the lifetime of an audio stack.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com
This is a continuation of a discussion that begun in the middle of 2022 [1] and was part of a larger series addressing several HDAudio topics.
Single rmmod on ASoC's codec driver module is enough to cause a panic. Given our results, no regression shows up with modprobe/rmmod on snd_hda_intel side with this patch applied.
sound/pci/hda/hda_codec.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index edd653ece70d..ac1cc7c5290e 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -795,7 +795,6 @@ void snd_hda_codec_cleanup_for_unbind(struct hda_codec *codec) snd_array_free(&codec->cvt_setups); snd_array_free(&codec->spdif_out); snd_array_free(&codec->verbs);
- codec->preset = NULL; codec->follower_dig_outs = NULL; codec->spdif_status_reset = 0; snd_array_free(&codec->mixers);
On 2023-01-17 4:48 PM, Pierre-Louis Bossart wrote:
On 1/17/23 09:47, Cezary Rojewski wrote:
Several functions that take part in codec's initialization and removal are re-used by ASoC codec drivers implementations. Drivers mimic the behavior of hda_codec_driver_probe/remove() found in sound/pci/hda/hda_bind.c with their component->probe/remove() instead.
One of the reasons for that is the expectation of snd_hda_codec_device_new() to receive a valid struct snd_card pointer what cannot be fulfilled on ASoC side until a card is attempted to be
very hard to follow. Is there a spurious 'what' to be removed? Or is there missing text? Please consider rewording with simpler sentences.
Thanks for the comments. 'what' is here on purpose as to my ear this sentence sounds reasonable, but I'm not a native English speaker so I might be wrong here.
The following is being explained by the commit message:
- functions such as snd_hda_codec_device_new() expect a valid pointer to struct snd_card instance - for ASoC hda codec drivers, when hdev_attach/detach() are called, there is no possibility to provide one to HDAudio API as card components are not yet enumerated - once component->probe() is invoked and succeeds, component->card will point to a valid sound card - hda codec driver is now ready to call snd_hda_codec_device_new()
bound and its component probing is triggered.
As ASoC sound card may be unbound without codec device being actually removed from the system, unsetting ->preset in snd_hda_codec_cleanup_for_unbind() interferes with module unload -> load scenario causing null-ptr-deref. Preset is assigned only once, during device/driver matching whereas ASoC codec driver's module reloading may occur several times throughout the lifetime of an audio stack.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com
On 2023-01-18 12:38 PM, Cezary Rojewski wrote:
On 2023-01-17 4:48 PM, Pierre-Louis Bossart wrote:
On 1/17/23 09:47, Cezary Rojewski wrote:
Several functions that take part in codec's initialization and removal are re-used by ASoC codec drivers implementations. Drivers mimic the behavior of hda_codec_driver_probe/remove() found in sound/pci/hda/hda_bind.c with their component->probe/remove() instead.
One of the reasons for that is the expectation of snd_hda_codec_device_new() to receive a valid struct snd_card pointer what cannot be fulfilled on ASoC side until a card is attempted to be
very hard to follow. Is there a spurious 'what' to be removed? Or is there missing text? Please consider rewording with simpler sentences.
Thanks for the comments. 'what' is here on purpose as to my ear this sentence sounds reasonable, but I'm not a native English speaker so I might be wrong here.
The following is being explained by the commit message:
- functions such as snd_hda_codec_device_new() expect a valid pointer to
struct snd_card instance
- for ASoC hda codec drivers, when hdev_attach/detach() are called,
there is no possibility to provide one to HDAudio API as card components are not yet enumerated
- once component->probe() is invoked and succeeds, component->card will
point to a valid sound card
- hda codec driver is now ready to call snd_hda_codec_device_new()
Let me rephrase the last 2 points: hda codec driver is ready to call functions such as snd_hda_codec_device_new() only when its component->probe() op gets invoked. Until that happens, field component->card is invalid.
On Tue, 17 Jan 2023 16:47:34 +0100, Cezary Rojewski wrote:
Several functions that take part in codec's initialization and removal are re-used by ASoC codec drivers implementations. Drivers mimic the behavior of hda_codec_driver_probe/remove() found in sound/pci/hda/hda_bind.c with their component->probe/remove() instead.
One of the reasons for that is the expectation of snd_hda_codec_device_new() to receive a valid struct snd_card pointer what cannot be fulfilled on ASoC side until a card is attempted to be bound and its component probing is triggered.
As ASoC sound card may be unbound without codec device being actually removed from the system, unsetting ->preset in snd_hda_codec_cleanup_for_unbind() interferes with module unload -> load scenario causing null-ptr-deref. Preset is assigned only once, during device/driver matching whereas ASoC codec driver's module reloading may occur several times throughout the lifetime of an audio stack.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com
This is a continuation of a discussion that begun in the middle of 2022 [1] and was part of a larger series addressing several HDAudio topics.
Single rmmod on ASoC's codec driver module is enough to cause a panic. Given our results, no regression shows up with modprobe/rmmod on snd_hda_intel side with this patch applied.
I think one possible regression by this change would be the case you reload another codec driver. With keeping codec->preset, it's still thought as if already matched, and a wrong one could be used.
And, this would be nothing but a leak of the possibly freed address. After hda_codec_driver_remove(), card->preset may point to an already freed address.
So, just removing isn't right. It has to be cleared somewhere instead, e.g. in hda_bind.c.
But, one thing I'm still concerned is that your comment about the call without the card binding. Do you mean that the snd_hda_codec_cleanup_for_unbind() may be called even if codec->card isn't set?
thanks,
Takashi
On 2023-01-17 5:01 PM, Takashi Iwai wrote:
...
This is a continuation of a discussion that begun in the middle of 2022 [1] and was part of a larger series addressing several HDAudio topics.
Single rmmod on ASoC's codec driver module is enough to cause a panic. Given our results, no regression shows up with modprobe/rmmod on snd_hda_intel side with this patch applied.
I think one possible regression by this change would be the case you reload another codec driver. With keeping codec->preset, it's still thought as if already matched, and a wrong one could be used.
And, this would be nothing but a leak of the possibly freed address. After hda_codec_driver_remove(), card->preset may point to an already freed address.
So, just removing isn't right. It has to be cleared somewhere instead, e.g. in hda_bind.c.
But, one thing I'm still concerned is that your comment about the call without the card binding. Do you mean that the snd_hda_codec_cleanup_for_unbind() may be called even if codec->card isn't set?
One proposition would be to add line: codec->preset = NULL;
after both invocations of snd_hda_codec_cleanup_for_unbind() in hda_codec_driver_probe/remove() in sound/pci/hda/hda_bind.c.
In regard to your last question - no, cleanup is only called either in component->probe()'s error path or in component->remove() once snd_hda_codec_device_new() succeeds and thus, codec->card points to a valid sound card.
What I meant by my comment, is that removal of any components of an ASoC sound card will cause all other components to have their ->remove() op called too. Let's say I unload snd_soc_avs_hdaudio module without unloading snd_soc_avs. As bus (snd_soc_avs) owns the codecs, its platform component->remove() gets called right after codec component->remove() but the actual devices are still present in the system even with the sound card module (snd_soc_avs_hdaudio) unloaded.
Regards, Czarek
On Wed, 18 Jan 2023 13:04:05 +0100, Cezary Rojewski wrote:
On 2023-01-17 5:01 PM, Takashi Iwai wrote:
...
This is a continuation of a discussion that begun in the middle of 2022 [1] and was part of a larger series addressing several HDAudio topics.
Single rmmod on ASoC's codec driver module is enough to cause a panic. Given our results, no regression shows up with modprobe/rmmod on snd_hda_intel side with this patch applied.
I think one possible regression by this change would be the case you reload another codec driver. With keeping codec->preset, it's still thought as if already matched, and a wrong one could be used.
And, this would be nothing but a leak of the possibly freed address. After hda_codec_driver_remove(), card->preset may point to an already freed address.
So, just removing isn't right. It has to be cleared somewhere instead, e.g. in hda_bind.c.
But, one thing I'm still concerned is that your comment about the call without the card binding. Do you mean that the snd_hda_codec_cleanup_for_unbind() may be called even if codec->card isn't set?
One proposition would be to add line: codec->preset = NULL;
after both invocations of snd_hda_codec_cleanup_for_unbind() in hda_codec_driver_probe/remove() in sound/pci/hda/hda_bind.c.
Yes, that's what I meant, too.
In regard to your last question - no, cleanup is only called either in component->probe()'s error path or in component->remove() once snd_hda_codec_device_new() succeeds and thus, codec->card points to a valid sound card.
What I meant by my comment, is that removal of any components of an ASoC sound card will cause all other components to have their ->remove() op called too. Let's say I unload snd_soc_avs_hdaudio module without unloading snd_soc_avs. As bus (snd_soc_avs) owns the codecs, its platform component->remove() gets called right after codec component->remove() but the actual devices are still present in the system even with the sound card module (snd_soc_avs_hdaudio) unloaded.
OK, then the snd_hda_codec object itself is kept bound on the bus, hence the call of snd_hda_codec_cleanup_for_unbind() is somehow inappropriate. The function could be renamed for avoid misunderstanding.
thanks,
Takashi
participants (3)
-
Cezary Rojewski -
Pierre-Louis Bossart -
Takashi Iwai