| Message ID | 20230117154734.950487-1-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ALSA: hda: Do not unset preset when cleaning up codec | expand |
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);
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 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 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
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);
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(-)