[PATCH] ALSA: hda: Do not unset preset when cleaning up codec

[PATCH] ALSA: hda: Do not unset preset when cleaning up codec

[Cezary Rojewski]

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@intel.com/

 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);
-- 
2.25.1

Re: [PATCH] ALSA: hda: Do not unset preset when cleaning up codec

[Pierre-Louis Bossart]

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.
> 
> [1]: https://lore.kernel.org/alsa-devel/20220706120230.427296-2-cezary.rojewski@intel.com/
> 
>  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);

Re: [PATCH] ALSA: hda: Do not unset preset when cleaning up codec

[Takashi Iwai]

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

Re: [PATCH] ALSA: hda: Do not unset preset when cleaning up codec

[Cezary Rojewski]

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>
>> ---

Re: [PATCH] ALSA: hda: Do not unset preset when cleaning up codec

[Cezary Rojewski]

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.

Re: [PATCH] ALSA: hda: Do not unset preset when cleaning up codec

[Cezary Rojewski]

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

Re: [PATCH] ALSA: hda: Do not unset preset when cleaning up codec

[Takashi Iwai]

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