[PATCH 0/9] ALSA: hda: Codec-reload bug fixes and cleanups

[PATCH 0/9] ALSA: hda: Codec-reload bug fixes and cleanups

[Cezary Rojewski]

Total of 6 fixes and 3 cleanups - cleanups are last.

All of the fixes address problems that present themselves in situation
when user engages in codec driver reload. Second condition to reproduce
is two-step initialization of HDAudio codec - this is the case only for
ASoC HDAudio bus driver as snd_hda_intel calls only compound function
snd_hda_codec_new(). Once these conditions are met, several
reload/unload scenarios end with null-ptr-deref and page faults. Goal of
the series is to allow codec/bus driver reloading without any errors.

Amadeusz Sławiński (2):
  ALSA: hda: Reset all SIE bits in INTCTL
  ALSA: hda: Remove unused macro definition

Cezary Rojewski (7):
  ALSA: hda: Do not unset preset when cleaning up codec
  ALSA: hda: Fix null-ptr-deref when i915 fails and hdmi is denylisted
  ALSA: hda: Make device usage_count consistent across subsequent
    probing
  ALSA: hda: Fix put_device() inconsistency in error path
  ALSA: hda: Skip event processing for unregistered codecs
  ALSA: hda: Fix page fault in snd_hda_codec_shutdown()
  ALSA: hda/realtek: Remove redundant init_hook() in alc_default_init()

 include/sound/hda_codec.h           |  1 -
 include/sound/hdaudio.h             |  1 +
 sound/hda/ext/hdac_ext_controller.c |  7 ---
 sound/hda/hdac_bus.c                |  2 +-
 sound/hda/hdac_controller.c         |  7 +--
 sound/pci/hda/hda_bind.c            |  7 +++
 sound/pci/hda/hda_codec.c           | 83 +++++++++++++++--------------
 sound/pci/hda/patch_realtek.c       |  3 --
 sound/soc/codecs/hda.c              |  4 +-
 9 files changed, 56 insertions(+), 59 deletions(-)

-- 
2.25.1

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

[Cezary Rojewski]

snd_hda_codec_cleanup_for_unbind() unsets preset what interferes with
module unloading and triggers null-ptr-deref. Preset is assigned only
once, during device/driver matching whereas module reload and unload
follow completely different path and may occur several times during
runtime.

Fixes: 9a6246ff78ac ("ALSA: hda - Implement unbind more safely")
Signed-off-by: Cezary Rojewski <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 7579a6982f47..9ceb642ac819 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 1/9] ALSA: hda: Do not unset preset when cleaning up codec

[Takashi Iwai]

On Wed, 06 Jul 2022 14:02:22 +0200,
Cezary Rojewski wrote:
> 
> snd_hda_codec_cleanup_for_unbind() unsets preset what interferes with
> module unloading and triggers null-ptr-deref. Preset is assigned only
> once, during device/driver matching whereas module reload and unload
> follow completely different path and may occur several times during
> runtime.

Hm, the driver reload/unload does unbind.  Keeping this field mean to
leave the pointer to the possibly freed object, no?

And if it's not cleared, where is this field cleared instead?


thanks,

Takashi

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

[Cezary Rojewski]

On 2022-07-09 6:34 PM, Takashi Iwai wrote:
> On Wed, 06 Jul 2022 14:02:22 +0200,
> Cezary Rojewski wrote:
>>
>> snd_hda_codec_cleanup_for_unbind() unsets preset what interferes with
>> module unloading and triggers null-ptr-deref. Preset is assigned only
>> once, during device/driver matching whereas module reload and unload
>> follow completely different path and may occur several times during
>> runtime.
> 
> Hm, the driver reload/unload does unbind.  Keeping this field mean to
> leave the pointer to the possibly freed object, no?
> 
> And if it's not cleared, where is this field cleared instead?


avs-driver i.e. the bus driver takes responsibility for the codec device 
only. There is no real probe(), just the device creation and 
initialization of its fields. The rest is handled by the component 
driver (sound/soc/hda.c). If this field is cleared and the test is 
limited to reloading HDAudio codec module alone, we get a panic. 
Something similar to the stack found below my message.

In regard to the other question - are presets freed at all? It seems all 
of them are part of the static device-driver matching list. If so, the 
pointer is always valid.


[  136.827856] RIP: 0010:hda_codec_probe+0x16c/0x560 [snd_soc_hda_codec]
[  136.827929] Code: ff 85 c0 0f 88 5b 0b 00 00 4d 8d bc 24 d0 03 00 00 
4c 89 ff e8 e5 a2 9e ca 49 8b 9c 24 d0 03 00 00 48 8d 7b 10 e8 d4 a2 9e 
ca <48> 8b 73 10 4c 89 e7 e8 e8 7d fb ff 85 c0 0f 88 43 0b 00 00 4c 89
[  136.828028] RSP: 0018:ffff888101af74d0 EFLAGS: 00010286
[  136.828079] RAX: 0000000000000001 RBX: 0000000000000000 RCX: 
ffffffff8b4f1b1a
[  136.828128] RDX: 0000000000000001 RSI: 0000000000000008 RDI: 
ffffffff8e323d20
[  136.828175] RBP: ffff888101af7540 R08: 1ffffffff1c647a4 R09: 
fffffbfff1c647a5
[  136.828224] R10: ffffffff8e323d27 R11: fffffbfff1c647a4 R12: 
ffff888102920000
[  136.828272] R13: ffff88810812e428 R14: ffff888102925028 R15: 
ffff8881029203d0
[  136.828323] FS:  00007f9049dd8540(0000) GS:ffff888227100000(0000) 
knlGS:0000000000000000
[  136.828380] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  136.828425] CR2: 0000000000000010 CR3: 000000010f086001 CR4: 
00000000003706e0
[  136.828474] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 
0000000000000000
[  136.828520] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 
0000000000000400
[  136.828568] Call Trace:
[  136.828593]  <TASK>
[  136.828628]  snd_soc_component_probe+0x3a/0x60 [snd_soc_core]
[  136.828981]  soc_probe_component+0x276/0x4a0 [snd_soc_core]
[  136.829274]  snd_soc_bind_card+0x819/0x13d0 [snd_soc_core]
[  136.829560]  ? __kasan_slab_alloc+0x32/0x90
[  136.829614]  snd_soc_register_card+0x24e/0x260 [snd_soc_core]
[  136.829900]  devm_snd_soc_register_card+0x48/0x90 [snd_soc_core]
[  136.830204]  avs_hdaudio_probe+0x298/0x2c0 [snd_soc_avs_hdaudio]
[  136.830269]  platform_probe+0x67/0x100
[  136.830313]  really_probe+0x1ff/0x500
[  136.830354]  __driver_probe_device+0xeb/0x240
[  136.830397]  driver_probe_device+0x4e/0xf0
[  136.830438]  __driver_attach+0xfd/0x210
[  136.830478]  ? __device_attach_driver+0x170/0x170
[  136.830520]  bus_for_each_dev+0xf9/0x150
[  136.830557]  ? subsys_dev_iter_exit+0x10/0x10
[  136.830597]  ? preempt_count_sub+0x18/0xc0
[  136.830643]  driver_attach+0x2d/0x40
[  136.830679]  bus_add_driver+0x28e/0x320
[  136.830722]  driver_register+0xdc/0x170
[  136.830763]  ? 0xffffffffc0428000
[  136.830796]  __platform_driver_register+0x39/0x40
[  136.830842]  avs_hdaudio_driver_init+0x1c/0x1000 [snd_soc_avs_hdaudio]
[  136.830902]  do_one_initcall+0xa0/0x2e0
[  136.830939]  ? initcall_blacklisted+0x170/0x170
[  136.830981]  ? __kasan_kmalloc+0x88/0xa0
[  136.831020]  ? kasan_poison+0x3c/0x50
[  136.831059]  ? kasan_unpoison+0x28/0x50
[  136.831100]  ? kasan_poison+0x3c/0x50
[  136.831139]  ? __asan_register_globals+0x5e/0x70
[  136.831187]  do_init_module+0xf6/0x350
[  136.831228]  load_module+0x2bf5/0x2e30
(...)

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

[Takashi Iwai]

On Mon, 11 Jul 2022 10:25:17 +0200,
Cezary Rojewski wrote:
> 
> On 2022-07-09 6:34 PM, Takashi Iwai wrote:
> > On Wed, 06 Jul 2022 14:02:22 +0200,
> > Cezary Rojewski wrote:
> >> 
> >> snd_hda_codec_cleanup_for_unbind() unsets preset what interferes with
> >> module unloading and triggers null-ptr-deref. Preset is assigned only
> >> once, during device/driver matching whereas module reload and unload
> >> follow completely different path and may occur several times during
> >> runtime.
> > 
> > Hm, the driver reload/unload does unbind.  Keeping this field mean to
> > leave the pointer to the possibly freed object, no?
> > 
> > And if it's not cleared, where is this field cleared instead?
> 
> 
> avs-driver i.e. the bus driver takes responsibility for the codec
> device only. There is no real probe(), just the device creation and
> initialization of its fields. The rest is handled by the component
> driver (sound/soc/hda.c). If this field is cleared and the test is
> limited to reloading HDAudio codec module alone, we get a
> panic. Something similar to the stack found below my message.
> 
> In regard to the other question - are presets freed at all? It seems
> all of them are part of the static device-driver matching list. If so,
> the pointer is always valid.

When the codec driver is unbound and the module is unloaded, the whole
objects and symbols are gone.


> [  136.827856] RIP: 0010:hda_codec_probe+0x16c/0x560 [snd_soc_hda_codec]
> [  136.827929] Code: ff 85 c0 0f 88 5b 0b 00 00 4d 8d bc 24 d0 03 00
> 00 4c 89 ff e8 e5 a2 9e ca 49 8b 9c 24 d0 03 00 00 48 8d 7b 10 e8 d4
> a2 9e ca <48> 8b 73 10 4c 89 e7 e8 e8 7d fb ff 85 c0 0f 88 43 0b 00 00
> 4c 89
> [  136.828028] RSP: 0018:ffff888101af74d0 EFLAGS: 00010286
> [  136.828079] RAX: 0000000000000001 RBX: 0000000000000000 RCX:
> ffffffff8b4f1b1a
> [  136.828128] RDX: 0000000000000001 RSI: 0000000000000008 RDI:
> ffffffff8e323d20
> [  136.828175] RBP: ffff888101af7540 R08: 1ffffffff1c647a4 R09:
> fffffbfff1c647a5
> [  136.828224] R10: ffffffff8e323d27 R11: fffffbfff1c647a4 R12:
> ffff888102920000
> [  136.828272] R13: ffff88810812e428 R14: ffff888102925028 R15:
> ffff8881029203d0
> [  136.828323] FS:  00007f9049dd8540(0000) GS:ffff888227100000(0000)
> knlGS:0000000000000000
> [  136.828380] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  136.828425] CR2: 0000000000000010 CR3: 000000010f086001 CR4:
> 00000000003706e0
> [  136.828474] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> 0000000000000000
> [  136.828520] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
> 0000000000000400
> [  136.828568] Call Trace:
> [  136.828593]  <TASK>
> [  136.828628]  snd_soc_component_probe+0x3a/0x60 [snd_soc_core]
> [  136.828981]  soc_probe_component+0x276/0x4a0 [snd_soc_core]
> [  136.829274]  snd_soc_bind_card+0x819/0x13d0 [snd_soc_core]
> [  136.829560]  ? __kasan_slab_alloc+0x32/0x90
> [  136.829614]  snd_soc_register_card+0x24e/0x260 [snd_soc_core]
> [  136.829900]  devm_snd_soc_register_card+0x48/0x90 [snd_soc_core]
> [  136.830204]  avs_hdaudio_probe+0x298/0x2c0 [snd_soc_avs_hdaudio]
> [  136.830269]  platform_probe+0x67/0x100
> [  136.830313]  really_probe+0x1ff/0x500
> [  136.830354]  __driver_probe_device+0xeb/0x240
> [  136.830397]  driver_probe_device+0x4e/0xf0
> [  136.830438]  __driver_attach+0xfd/0x210
> [  136.830478]  ? __device_attach_driver+0x170/0x170
> [  136.830520]  bus_for_each_dev+0xf9/0x150
> [  136.830557]  ? subsys_dev_iter_exit+0x10/0x10
> [  136.830597]  ? preempt_count_sub+0x18/0xc0
> [  136.830643]  driver_attach+0x2d/0x40
> [  136.830679]  bus_add_driver+0x28e/0x320
> [  136.830722]  driver_register+0xdc/0x170
> [  136.830763]  ? 0xffffffffc0428000
> [  136.830796]  __platform_driver_register+0x39/0x40
> [  136.830842]  avs_hdaudio_driver_init+0x1c/0x1000 [snd_soc_avs_hdaudio]
> [  136.830902]  do_one_initcall+0xa0/0x2e0
> [  136.830939]  ? initcall_blacklisted+0x170/0x170
> [  136.830981]  ? __kasan_kmalloc+0x88/0xa0
> [  136.831020]  ? kasan_poison+0x3c/0x50
> [  136.831059]  ? kasan_unpoison+0x28/0x50
> [  136.831100]  ? kasan_poison+0x3c/0x50
> [  136.831139]  ? __asan_register_globals+0x5e/0x70
> [  136.831187]  do_init_module+0xf6/0x350
> [  136.831228]  load_module+0x2bf5/0x2e30
> (...)

Hmm, in the Oops above, at which moment,
snd_hda_codec_cleanup_for_unbind() is called via which function?
Is it the unload of HD-audio codec driver during the probe of AVS
HD-audio?

The preset is assigned to the given HD-audio device object for the
attached codec driver.  Once after the codec driver gets unbound, you
must not access to this codec driver's methods any longer, hence we
clear the preset field.

So I wonder how the access to the codec->preset happens after the
codec unbind.


thanks,

Takashi

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

[Cezary Rojewski]

On 2022-07-11 4:12 PM, Takashi Iwai wrote:
> On Mon, 11 Jul 2022 10:25:17 +0200,
> Cezary Rojewski wrote:

...

>> avs-driver i.e. the bus driver takes responsibility for the codec
>> device only. There is no real probe(), just the device creation and
>> initialization of its fields. The rest is handled by the component
>> driver (sound/soc/hda.c). If this field is cleared and the test is
>> limited to reloading HDAudio codec module alone, we get a
>> panic. Something similar to the stack found below my message.
>>
>> In regard to the other question - are presets freed at all? It seems
>> all of them are part of the static device-driver matching list. If so,
>> the pointer is always valid.
> 
> When the codec driver is unbound and the module is unloaded, the whole
> objects and symbols are gone.


hda_codec_driver_remove() won't get even called when soc-card is being 
unbound so everything is still here.

>> [  136.827856] RIP: 0010:hda_codec_probe+0x16c/0x560 [snd_soc_hda_codec]

>> [  136.828568] Call Trace:
>> [  136.828593]  <TASK>
>> [  136.828628]  snd_soc_component_probe+0x3a/0x60 [snd_soc_core]
>> [  136.828981]  soc_probe_component+0x276/0x4a0 [snd_soc_core]
>> [  136.829274]  snd_soc_bind_card+0x819/0x13d0 [snd_soc_core]
>> [  136.829560]  ? __kasan_slab_alloc+0x32/0x90
>> [  136.829614]  snd_soc_register_card+0x24e/0x260 [snd_soc_core]
>> [  136.829900]  devm_snd_soc_register_card+0x48/0x90 [snd_soc_core]
>> [  136.830204]  avs_hdaudio_probe+0x298/0x2c0 [snd_soc_avs_hdaudio]

>> (...)
> 
> Hmm, in the Oops above, at which moment,
> snd_hda_codec_cleanup_for_unbind() is called via which function?
> Is it the unload of HD-audio codec driver during the probe of AVS
> HD-audio?
> 
> The preset is assigned to the given HD-audio device object for the
> attached codec driver.  Once after the codec driver gets unbound, you
> must not access to this codec driver's methods any longer, hence we
> clear the preset field.
> 
> So I wonder how the access to the codec->preset happens after the
> codec unbind.


Test scenario:
- enumerate avs-driver stack on machine with HDAudio codec present
- rmmod snd_soc_avs_hdaudio // just the machine board driver i.e. 
soc-card driver
- modprobe snd_soc_avs_hdaudio
 >>> panic <<<

snd_hda_codec_cleanup_for_unbind() is called in more places than just 
HDAudio codec driver's probe() and remove(). It's also called whenever 
HDAudio codec soc-component is being removed. Relevant part of the stack 
showing when does the cleanup function get called during rmmod:

[  220.549349]  snd_hda_codec_cleanup_for_unbind+0x25/0x451 [snd_hda_codec]
[  220.549536]  ? dump_stack_lvl+0x45/0x49
[  220.549568]  hda_codec_remove.cold+0x14/0x138 [snd_soc_hda_codec]
[  220.549609]  snd_soc_component_remove+0x34/0x40 [snd_soc_core]
[  220.549942]  soc_remove_component+0x113/0x120 [snd_soc_core]
[  220.550249]  soc_cleanup_card_resources+0x1a7/0x4a0 [snd_soc_core]
[  220.550561]  snd_soc_unbind_card+0x9e/0x190 [snd_soc_core]
[  220.550885]  snd_soc_unregister_card+0x28/0x80 [snd_soc_core]
[  220.551193]  devm_card_release+0x1d/0x20 [snd_soc_core]
[  220.551527]  release_nodes+0x73/0x170
[  220.551549]  ? preempt_count_sub+0x18/0xc0
[  220.551576]  devres_release_all+0x10a/0x150
[  220.551600]  ? devres_remove_group+0x260/0x260
[  220.551630]  device_unbind_cleanup+0x14/0xd0
[  220.551656]  device_release_driver_internal+0x146/0x1d0
[  220.551688]  driver_detach+0x81/0xf0
[  220.551716]  bus_remove_driver+0xae/0x170
[  220.551743]  driver_unregister+0x4d/0x70
[  220.551770]  platform_driver_unregister+0x12/0x20
[  220.551799]  avs_hdaudio_driver_exit+0x10/0x12 [snd_soc_avs_hdaudio]


Regards,
Czarek

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

[Takashi Iwai]

On Tue, 12 Jul 2022 11:42:56 +0200,
Cezary Rojewski wrote:
> 
> On 2022-07-11 4:12 PM, Takashi Iwai wrote:
> > On Mon, 11 Jul 2022 10:25:17 +0200,
> > Cezary Rojewski wrote:
> 
> ...
> 
> >> avs-driver i.e. the bus driver takes responsibility for the codec
> >> device only. There is no real probe(), just the device creation and
> >> initialization of its fields. The rest is handled by the component
> >> driver (sound/soc/hda.c). If this field is cleared and the test is
> >> limited to reloading HDAudio codec module alone, we get a
> >> panic. Something similar to the stack found below my message.
> >> 
> >> In regard to the other question - are presets freed at all? It seems
> >> all of them are part of the static device-driver matching list. If so,
> >> the pointer is always valid.
> > 
> > When the codec driver is unbound and the module is unloaded, the whole
> > objects and symbols are gone.
> 
> 
> hda_codec_driver_remove() won't get even called when soc-card is being
> unbound so everything is still here.
> 
> >> [  136.827856] RIP: 0010:hda_codec_probe+0x16c/0x560 [snd_soc_hda_codec]
> 
> >> [  136.828568] Call Trace:
> >> [  136.828593]  <TASK>
> >> [  136.828628]  snd_soc_component_probe+0x3a/0x60 [snd_soc_core]
> >> [  136.828981]  soc_probe_component+0x276/0x4a0 [snd_soc_core]
> >> [  136.829274]  snd_soc_bind_card+0x819/0x13d0 [snd_soc_core]
> >> [  136.829560]  ? __kasan_slab_alloc+0x32/0x90
> >> [  136.829614]  snd_soc_register_card+0x24e/0x260 [snd_soc_core]
> >> [  136.829900]  devm_snd_soc_register_card+0x48/0x90 [snd_soc_core]
> >> [  136.830204]  avs_hdaudio_probe+0x298/0x2c0 [snd_soc_avs_hdaudio]
> 
> >> (...)
> > 
> > Hmm, in the Oops above, at which moment,
> > snd_hda_codec_cleanup_for_unbind() is called via which function?
> > Is it the unload of HD-audio codec driver during the probe of AVS
> > HD-audio?
> > 
> > The preset is assigned to the given HD-audio device object for the
> > attached codec driver.  Once after the codec driver gets unbound, you
> > must not access to this codec driver's methods any longer, hence we
> > clear the preset field.
> > 
> > So I wonder how the access to the codec->preset happens after the
> > codec unbind.
> 
> 
> Test scenario:
> - enumerate avs-driver stack on machine with HDAudio codec present
> - rmmod snd_soc_avs_hdaudio // just the machine board driver
> i.e. soc-card driver
> - modprobe snd_soc_avs_hdaudio
> >>> panic <<<
> 
> snd_hda_codec_cleanup_for_unbind() is called in more places than just
> HDAudio codec driver's probe() and remove(). It's also called whenever
> HDAudio codec soc-component is being removed. Relevant part of the
> stack showing when does the cleanup function get called during rmmod:
> 
> [  220.549349]  snd_hda_codec_cleanup_for_unbind+0x25/0x451 [snd_hda_codec]
> [  220.549536]  ? dump_stack_lvl+0x45/0x49
> [  220.549568]  hda_codec_remove.cold+0x14/0x138 [snd_soc_hda_codec]
> [  220.549609]  snd_soc_component_remove+0x34/0x40 [snd_soc_core]
> [  220.549942]  soc_remove_component+0x113/0x120 [snd_soc_core]
> [  220.550249]  soc_cleanup_card_resources+0x1a7/0x4a0 [snd_soc_core]
> [  220.550561]  snd_soc_unbind_card+0x9e/0x190 [snd_soc_core]
> [  220.550885]  snd_soc_unregister_card+0x28/0x80 [snd_soc_core]
> [  220.551193]  devm_card_release+0x1d/0x20 [snd_soc_core]
> [  220.551527]  release_nodes+0x73/0x170
> [  220.551549]  ? preempt_count_sub+0x18/0xc0
> [  220.551576]  devres_release_all+0x10a/0x150
> [  220.551600]  ? devres_remove_group+0x260/0x260
> [  220.551630]  device_unbind_cleanup+0x14/0xd0
> [  220.551656]  device_release_driver_internal+0x146/0x1d0
> [  220.551688]  driver_detach+0x81/0xf0
> [  220.551716]  bus_remove_driver+0xae/0x170
> [  220.551743]  driver_unregister+0x4d/0x70
> [  220.551770]  platform_driver_unregister+0x12/0x20
> [  220.551799]  avs_hdaudio_driver_exit+0x10/0x12 [snd_soc_avs_hdaudio]

So, IMO,  you're scratching a wrong surface.  The problem is rather
that snd_hda_codec_cleanup_for_unbind() is called even if it's not for
unbinding the codec.


Takashi

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

[Cezary Rojewski]

On 2022-07-12 12:46 PM, Takashi Iwai wrote:
> On Tue, 12 Jul 2022 11:42:56 +0200,
> Cezary Rojewski wrote:

...

>> snd_hda_codec_cleanup_for_unbind() is called in more places than just
>> HDAudio codec driver's probe() and remove(). It's also called whenever
>> HDAudio codec soc-component is being removed. Relevant part of the
>> stack showing when does the cleanup function get called during rmmod:
>>
>> [  220.549349]  snd_hda_codec_cleanup_for_unbind+0x25/0x451 [snd_hda_codec]
>> [  220.549536]  ? dump_stack_lvl+0x45/0x49
>> [  220.549568]  hda_codec_remove.cold+0x14/0x138 [snd_soc_hda_codec]
>> [  220.549609]  snd_soc_component_remove+0x34/0x40 [snd_soc_core]
>> [  220.549942]  soc_remove_component+0x113/0x120 [snd_soc_core]
>> [  220.550249]  soc_cleanup_card_resources+0x1a7/0x4a0 [snd_soc_core]
>> [  220.550561]  snd_soc_unbind_card+0x9e/0x190 [snd_soc_core]
>> [  220.550885]  snd_soc_unregister_card+0x28/0x80 [snd_soc_core]
>> [  220.551193]  devm_card_release+0x1d/0x20 [snd_soc_core]
>> [  220.551527]  release_nodes+0x73/0x170
>> [  220.551549]  ? preempt_count_sub+0x18/0xc0
>> [  220.551576]  devres_release_all+0x10a/0x150
>> [  220.551600]  ? devres_remove_group+0x260/0x260
>> [  220.551630]  device_unbind_cleanup+0x14/0xd0
>> [  220.551656]  device_release_driver_internal+0x146/0x1d0
>> [  220.551688]  driver_detach+0x81/0xf0
>> [  220.551716]  bus_remove_driver+0xae/0x170
>> [  220.551743]  driver_unregister+0x4d/0x70
>> [  220.551770]  platform_driver_unregister+0x12/0x20
>> [  220.551799]  avs_hdaudio_driver_exit+0x10/0x12 [snd_soc_avs_hdaudio]
> 
> So, IMO,  you're scratching a wrong surface.  The problem is rather
> that snd_hda_codec_cleanup_for_unbind() is called even if it's not for
> unbinding the codec.


This is how ASoC HDAudio codec component was behaving for years, see 
sound/soc/codecs/hdac_hda.c. If the intention is _not_ do call 
snd_hda_codec_cleanup_for_unbind() then the teardown procedure will 
probably need a little update. Actually.. I'm afraid the init one would 
need an update to. Given how the implementation of HDAudio codec 
component's probe() and remove() looks like, there is no dropping the 
cleanup function without changing the upward path accordingly.

Well, to be honest the init/free procedures of HDAudio codec are a 
little hairy, perhaps it's time to address this.


Regards,
Czarek

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

[Takashi Iwai]

On Tue, 12 Jul 2022 12:58:09 +0200,
Cezary Rojewski wrote:
> 
> On 2022-07-12 12:46 PM, Takashi Iwai wrote:
> > On Tue, 12 Jul 2022 11:42:56 +0200,
> > Cezary Rojewski wrote:
> 
> ...
> 
> >> snd_hda_codec_cleanup_for_unbind() is called in more places than just
> >> HDAudio codec driver's probe() and remove(). It's also called whenever
> >> HDAudio codec soc-component is being removed. Relevant part of the
> >> stack showing when does the cleanup function get called during rmmod:
> >> 
> >> [  220.549349]  snd_hda_codec_cleanup_for_unbind+0x25/0x451 [snd_hda_codec]
> >> [  220.549536]  ? dump_stack_lvl+0x45/0x49
> >> [  220.549568]  hda_codec_remove.cold+0x14/0x138 [snd_soc_hda_codec]
> >> [  220.549609]  snd_soc_component_remove+0x34/0x40 [snd_soc_core]
> >> [  220.549942]  soc_remove_component+0x113/0x120 [snd_soc_core]
> >> [  220.550249]  soc_cleanup_card_resources+0x1a7/0x4a0 [snd_soc_core]
> >> [  220.550561]  snd_soc_unbind_card+0x9e/0x190 [snd_soc_core]
> >> [  220.550885]  snd_soc_unregister_card+0x28/0x80 [snd_soc_core]
> >> [  220.551193]  devm_card_release+0x1d/0x20 [snd_soc_core]
> >> [  220.551527]  release_nodes+0x73/0x170
> >> [  220.551549]  ? preempt_count_sub+0x18/0xc0
> >> [  220.551576]  devres_release_all+0x10a/0x150
> >> [  220.551600]  ? devres_remove_group+0x260/0x260
> >> [  220.551630]  device_unbind_cleanup+0x14/0xd0
> >> [  220.551656]  device_release_driver_internal+0x146/0x1d0
> >> [  220.551688]  driver_detach+0x81/0xf0
> >> [  220.551716]  bus_remove_driver+0xae/0x170
> >> [  220.551743]  driver_unregister+0x4d/0x70
> >> [  220.551770]  platform_driver_unregister+0x12/0x20
> >> [  220.551799]  avs_hdaudio_driver_exit+0x10/0x12 [snd_soc_avs_hdaudio]
> > 
> > So, IMO,  you're scratching a wrong surface.  The problem is rather
> > that snd_hda_codec_cleanup_for_unbind() is called even if it's not for
> > unbinding the codec.
> 
> 
> This is how ASoC HDAudio codec component was behaving for years, see
> sound/soc/codecs/hdac_hda.c. If the intention is _not_ do call
> snd_hda_codec_cleanup_for_unbind() then the teardown procedure will
> probably need a little update.

Do we see a similar crash with the hdac-hda stuff, too?

And, after avs_hdaudio_driver_exit() is called, why the codec object
still remains bound with the HD-audio (Realtek or whatever) codec
driver?

> Actually.. I'm afraid the init one
> would need an update to. Given how the implementation of HDAudio codec
> component's probe() and remove() looks like, there is no dropping the
> cleanup function without changing the upward path accordingly.
> 
> Well, to be honest the init/free procedures of HDAudio codec are a
> little hairy, perhaps it's time to address this.

Admittedly, the plumbing work for ASoC HD-audio was somewhat messy,
and it's fine if we can clean things up.

snd_hda_codec_cleanup_for_unbind() is certainly written for unbinding
the codec driver, and if a part of that function code is needed for
different purposes, it should be factored out properly, at least.


thanks,

Takashi

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

[Cezary Rojewski]

On 2022-07-15 4:55 PM, Takashi Iwai wrote:
> On Tue, 12 Jul 2022 12:58:09 +0200, Cezary Rojewski wrote:

>> This is how ASoC HDAudio codec component was behaving for years, see
>> sound/soc/codecs/hdac_hda.c. If the intention is _not_ do call
>> snd_hda_codec_cleanup_for_unbind() then the teardown procedure will
>> probably need a little update.
> 
> Do we see a similar crash with the hdac-hda stuff, too?
> 
> And, after avs_hdaudio_driver_exit() is called, why the codec object
> still remains bound with the HD-audio (Realtek or whatever) codec
> driver?


Hello Takashi,

Your reply was somehow missed by me and shows as a review for patch 5/9 
in my email-client. Sorry for the delay.

In regard to the hdac_hda.c question, we did test reloading for the 
skylake-driver and there are several places where the driver can cause 
panics, that is, it may not even get to hdac_hda failing - some other 
panic will pop up faster.

But yes, the exact same problem exists there as both implementations 
handle hdev_attach/detach() and component's probe/remove() is similar 
fashion.

>> Actually.. I'm afraid the init one
>> would need an update to. Given how the implementation of HDAudio codec
>> component's probe() and remove() looks like, there is no dropping the
>> cleanup function without changing the upward path accordingly.
>>
>> Well, to be honest the init/free procedures of HDAudio codec are a
>> little hairy, perhaps it's time to address this.
> 
> Admittedly, the plumbing work for ASoC HD-audio was somewhat messy,
> and it's fine if we can clean things up.
> 
> snd_hda_codec_cleanup_for_unbind() is certainly written for unbinding
> the codec driver, and if a part of that function code is needed for
> different purposes, it should be factored out properly, at least.

On ASoC side, component->probe() and component->remove() basically mimic 
the behavior of hda_codec_driver_probe/remove() found in 
sound/pci/hda/hda_bind.c. As ASoC sound card may be unbound without 
codec device being actually removed from the system, relying solely on 
driver's (not component's) probe/remove() may not be an option.

So, the discussion does not circle around just 
snd_hda_codec_cleanup_for_unbind() but basically any function that takes 
part in driver's probe() and remove() routines.

Right now, we are in a situation where user can generate a panic with a 
single rmmod. Also, our tests show no regression with modprobe/rmmod on 
snd_hda_intel side with this patch applied.


Regards,
Czarek

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

[Takashi Iwai]

On Tue, 17 Jan 2023 15:45:17 +0100,
Cezary Rojewski wrote:
> 
> On 2022-07-15 4:55 PM, Takashi Iwai wrote:
> > On Tue, 12 Jul 2022 12:58:09 +0200, Cezary Rojewski wrote:
> 
> >> This is how ASoC HDAudio codec component was behaving for years, see
> >> sound/soc/codecs/hdac_hda.c. If the intention is _not_ do call
> >> snd_hda_codec_cleanup_for_unbind() then the teardown procedure will
> >> probably need a little update.
> > 
> > Do we see a similar crash with the hdac-hda stuff, too?
> > 
> > And, after avs_hdaudio_driver_exit() is called, why the codec object
> > still remains bound with the HD-audio (Realtek or whatever) codec
> > driver?
> 
> 
> Hello Takashi,
> 
> Your reply was somehow missed by me and shows as a review for patch
> 5/9 in my email-client. Sorry for the delay.
> 
> In regard to the hdac_hda.c question, we did test reloading for the
> skylake-driver and there are several places where the driver can cause
> panics, that is, it may not even get to hdac_hda failing - some other
> panic will pop up faster.
> 
> But yes, the exact same problem exists there as both implementations
> handle hdev_attach/detach() and component's probe/remove() is similar
> fashion.
> 
> >> Actually.. I'm afraid the init one
> >> would need an update to. Given how the implementation of HDAudio codec
> >> component's probe() and remove() looks like, there is no dropping the
> >> cleanup function without changing the upward path accordingly.
> >> 
> >> Well, to be honest the init/free procedures of HDAudio codec are a
> >> little hairy, perhaps it's time to address this.
> > 
> > Admittedly, the plumbing work for ASoC HD-audio was somewhat messy,
> > and it's fine if we can clean things up.
> > 
> > snd_hda_codec_cleanup_for_unbind() is certainly written for unbinding
> > the codec driver, and if a part of that function code is needed for
> > different purposes, it should be factored out properly, at least.
> 
> On ASoC side, component->probe() and component->remove() basically
> mimic the behavior of hda_codec_driver_probe/remove() found in
> sound/pci/hda/hda_bind.c. As ASoC sound card may be unbound without
> codec device being actually removed from the system, relying solely on
> driver's (not component's) probe/remove() may not be an option.
> 
> So, the discussion does not circle around just
> snd_hda_codec_cleanup_for_unbind() but basically any function that
> takes part in driver's probe() and remove() routines.
> 
> Right now, we are in a situation where user can generate a panic with
> a single rmmod. Also, our tests show no regression with modprobe/rmmod
> on snd_hda_intel side with this patch applied.

Let's focus on that bug, then.  Can we restart the thread with the
minimal change?  Otherwise it's hard to review and discuss further.


thanks,

Takashi

[PATCH 2/9] ALSA: hda: Fix null-ptr-deref when i915 fails and hdmi is denylisted

[Cezary Rojewski]

If snd_hda_hdmi_codec module is denylisted and any event causes i915
enumeration to fail, is_likely_hdmi_codec() ends in null-ptr-deref.

As snd_soc_hda is an ASoC-based driver, its initialization is delayed
until all the necessary components appear in the system - allowing
actual sound card to enumerate. snd_hda_codec_configure() gets called by
the avs-driver core during probe_codecs() but the
snd_hda_codec_device_new(), necessary to complete codecs initialization,
happens only when codec-component of hda sound card is being probed.

Denylisting snd_hda_codec_hdmi module causes snd_hda_codec_configure()
to reach: codec_bind_generic() -> is_likely_hdmi_codec() which makes use
of ->wcaps and at this point the it isn't initialized yet - again,
requires completion of snd_hda_codec_device_new().

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/pci/hda/hda_bind.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/sound/pci/hda/hda_bind.c b/sound/pci/hda/hda_bind.c
index c572fb5886d5..cae9a975cbcc 100644
--- a/sound/pci/hda/hda_bind.c
+++ b/sound/pci/hda/hda_bind.c
@@ -248,6 +248,13 @@ static bool is_likely_hdmi_codec(struct hda_codec *codec)
 {
 	hda_nid_t nid;
 
+	/*
+	 * For ASoC users, if snd_hda_hdmi_codec module is denylisted and any
+	 * event causes i915 enumeration to fail, ->wcaps remains uninitialized.
+	 */
+	if (!codec->wcaps)
+		return true;
+
 	for_each_hda_codec_node(nid, codec) {
 		unsigned int wcaps = get_wcaps(codec, nid);
 		switch (get_wcaps_type(wcaps)) {
-- 
2.25.1

[PATCH 3/9] ALSA: hda: Make device usage_count consistent across subsequent probing

[Cezary Rojewski]

AVS HDAudio bus driver does not tie with codec drivers tighly and
snd_hda_codec_device_new() can be called after codec's module reload. In
such case, rpm is forbidden and invoking pm_runtime_forbid()
unconditionally causes device's usage_count to become unbalanced. This
is later caught by WARN_ON() found in sound/soc/hda.c. Detect such
circumstance and bump the usage_count instead.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/pci/hda/hda_codec.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 9ceb642ac819..83d954ab056f 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -1044,8 +1044,14 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
 			goto error;
 	}
 
+#ifdef CONFIG_PM
 	/* PM runtime needs to be enabled later after binding codec */
-	pm_runtime_forbid(&codec->core.dev);
+	if (codec->core.dev.power.runtime_auto)
+		pm_runtime_forbid(&codec->core.dev);
+	else
+		/* Keep the usage_count consistent across subsequent probing */
+		pm_runtime_get_noresume(&codec->core.dev);
+#endif
 
 	return 0;
 
-- 
2.25.1

[PATCH 4/9] ALSA: hda: Fix put_device() inconsistency in error path

[Cezary Rojewski]

AVS HDAudio bus driver does not tie with codec drivers tighly. Codec
device and its respective driver cleanup procedures are split and may
not occur one after the other. Device cleanup is performed only on
snd_hdac_ext_bus_device_remove() i.e. it's the bus driver's
responsibility. If codec component probing fails, put_device() found in
snd_hda_codec_device_new() may lead to page fault. Relocate it to
snd_hda_codec_new() to address the problem on ASoC side while keeping
status quo for snd_hda_intel.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/pci/hda/hda_codec.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 83d954ab056f..2381aced492f 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -949,6 +949,7 @@ int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card,
 		      unsigned int codec_addr, struct hda_codec **codecp)
 {
 	struct hda_codec *codec;
+	int ret;
 
 	codec = snd_hda_codec_device_init(bus, codec_addr, "hdaudioC%dD%d",
 					  card->number, codec_addr);
@@ -956,7 +957,11 @@ int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card,
 		return PTR_ERR(codec);
 	*codecp = codec;
 
-	return snd_hda_codec_device_new(bus, card, codec_addr, *codecp, true);
+	ret = snd_hda_codec_device_new(bus, card, codec_addr, *codecp, true);
+	if (ret)
+		put_device(hda_codec_dev(*codecp));
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(snd_hda_codec_new);
 
@@ -1011,19 +1016,17 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
 
 	if (codec->bus->modelname) {
 		codec->modelname = kstrdup(codec->bus->modelname, GFP_KERNEL);
-		if (!codec->modelname) {
-			err = -ENOMEM;
-			goto error;
-		}
+		if (!codec->modelname)
+			return -ENOMEM;
 	}
 
 	fg = codec->core.afg ? codec->core.afg : codec->core.mfg;
 	err = read_widget_caps(codec, fg);
 	if (err < 0)
-		goto error;
+		return err;
 	err = read_pin_defaults(codec);
 	if (err < 0)
-		goto error;
+		return err;
 
 	/* power-up all before initialization */
 	hda_set_power_state(codec, AC_PWRST_D0);
@@ -1041,7 +1044,7 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
 		/* ASoC features component management instead */
 		err = snd_device_new(card, SNDRV_DEV_CODEC, codec, &dev_ops);
 		if (err < 0)
-			goto error;
+			return err;
 	}
 
 #ifdef CONFIG_PM
@@ -1054,10 +1057,6 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
 #endif
 
 	return 0;
-
- error:
-	put_device(hda_codec_dev(codec));
-	return err;
 }
 EXPORT_SYMBOL_GPL(snd_hda_codec_device_new);
 
-- 
2.25.1

[PATCH 5/9] ALSA: hda: Skip event processing for unregistered codecs

[Cezary Rojewski]

When codec is unbound but not yet removed, in the eyes of
snd_hdac_bus_process_unsol_events() it is still a valid target to
delegate work to. Such behaviour may lead to use-after-free errors.
Address by verifying if codec is actually registered.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 include/sound/hda_codec.h |  1 -
 include/sound/hdaudio.h   |  1 +
 sound/hda/hdac_bus.c      |  2 +-
 sound/pci/hda/hda_codec.c | 10 +++++-----
 sound/soc/codecs/hda.c    |  4 ++--
 5 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h
index b7be300b6b18..6d3c82c4b6ac 100644
--- a/include/sound/hda_codec.h
+++ b/include/sound/hda_codec.h
@@ -231,7 +231,6 @@ struct hda_codec {
 	/* misc flags */
 	unsigned int configured:1; /* codec was configured */
 	unsigned int in_freeing:1; /* being released */
-	unsigned int registered:1; /* codec was registered */
 	unsigned int display_power_control:1; /* needs display power */
 	unsigned int spdif_status_reset :1; /* needs to toggle SPDIF for each
 					     * status change
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 15f15075238d..797bf67a164d 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -93,6 +93,7 @@ struct hdac_device {
 	bool lazy_cache:1;	/* don't wake up for writes */
 	bool caps_overwriting:1; /* caps overwrite being in process */
 	bool cache_coef:1;	/* cache COEF read/write too */
+	unsigned int registered:1; /* codec was registered */
 };
 
 /* device/driver type used for matching */
diff --git a/sound/hda/hdac_bus.c b/sound/hda/hdac_bus.c
index 71db8592b33d..d497414a5538 100644
--- a/sound/hda/hdac_bus.c
+++ b/sound/hda/hdac_bus.c
@@ -183,7 +183,7 @@ static void snd_hdac_bus_process_unsol_events(struct work_struct *work)
 		if (!(caddr & (1 << 4))) /* no unsolicited event? */
 			continue;
 		codec = bus->caddr_tbl[caddr & 0x0f];
-		if (!codec || !codec->dev.driver)
+		if (!codec || !codec->registered)
 			continue;
 		spin_unlock_irq(&bus->reg_lock);
 		drv = drv_to_hdac_driver(codec->dev.driver);
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 2381aced492f..75e85bf58681 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -772,11 +772,11 @@ static void codec_release_pcms(struct hda_codec *codec)
  */
 void snd_hda_codec_cleanup_for_unbind(struct hda_codec *codec)
 {
-	if (codec->registered) {
+	if (codec->core.registered) {
 		/* pm_runtime_put() is called in snd_hdac_device_exit() */
 		pm_runtime_get_noresume(hda_codec_dev(codec));
 		pm_runtime_disable(hda_codec_dev(codec));
-		codec->registered = 0;
+		codec->core.registered = 0;
 	}
 
 	snd_hda_codec_disconnect_pcms(codec);
@@ -824,14 +824,14 @@ void snd_hda_codec_display_power(struct hda_codec *codec, bool enable)
  */
 void snd_hda_codec_register(struct hda_codec *codec)
 {
-	if (codec->registered)
+	if (codec->core.registered)
 		return;
 	if (device_is_registered(hda_codec_dev(codec))) {
 		snd_hda_codec_display_power(codec, true);
 		pm_runtime_enable(hda_codec_dev(codec));
 		/* it was powered up in snd_hda_codec_new(), now all done */
 		snd_hda_power_down(codec);
-		codec->registered = 1;
+		codec->core.registered = 1;
 	}
 }
 EXPORT_SYMBOL_GPL(snd_hda_codec_register);
@@ -3047,7 +3047,7 @@ void snd_hda_codec_shutdown(struct hda_codec *codec)
 	struct hda_pcm *cpcm;
 
 	/* Skip the shutdown if codec is not registered */
-	if (!codec->registered)
+	if (!codec->core.registered)
 		return;
 
 	cancel_delayed_work_sync(&codec->jackpoll_work);
diff --git a/sound/soc/codecs/hda.c b/sound/soc/codecs/hda.c
index edcb8bc6806b..ad20a3dff9b7 100644
--- a/sound/soc/codecs/hda.c
+++ b/sound/soc/codecs/hda.c
@@ -274,7 +274,7 @@ static void hda_codec_remove(struct snd_soc_component *component)
 	struct hdac_device *hdev = &codec->core;
 	struct hdac_bus *bus = hdev->bus;
 	struct hdac_ext_link *hlink;
-	bool was_registered = codec->registered;
+	bool was_registered = codec->core.registered;
 
 	/* Don't allow any more runtime suspends */
 	pm_runtime_forbid(&hdev->dev);
@@ -376,7 +376,7 @@ static int hda_hdev_detach(struct hdac_device *hdev)
 {
 	struct hda_codec *codec = dev_to_hda_codec(&hdev->dev);
 
-	if (codec->registered)
+	if (codec->core.registered)
 		cancel_delayed_work_sync(&codec->jackpoll_work);
 
 	snd_soc_unregister_component(&hdev->dev);
-- 
2.25.1

Re: [PATCH 5/9] ALSA: hda: Skip event processing for unregistered codecs

[Takashi Iwai]

On Wed, 06 Jul 2022 14:02:26 +0200,
Cezary Rojewski wrote:
> 
> When codec is unbound but not yet removed, in the eyes of
> snd_hdac_bus_process_unsol_events() it is still a valid target to
> delegate work to. Such behaviour may lead to use-after-free errors.
> Address by verifying if codec is actually registered.
> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  include/sound/hda_codec.h |  1 -
>  include/sound/hdaudio.h   |  1 +
>  sound/hda/hdac_bus.c      |  2 +-
>  sound/pci/hda/hda_codec.c | 10 +++++-----
>  sound/soc/codecs/hda.c    |  4 ++--

This patch doesn't apply cleanly because the change in
sound/soc/codecs/hda.c still not included in my tree.

Mark, there's yet one case we need the merge of asoc-next :)


thanks,

Takashi

Re: [PATCH 5/9] ALSA: hda: Skip event processing for unregistered codecs

[Takashi Iwai]

On Sat, 09 Jul 2022 18:47:41 +0200,
Takashi Iwai wrote:
> 
> On Wed, 06 Jul 2022 14:02:26 +0200,
> Cezary Rojewski wrote:
> > 
> > When codec is unbound but not yet removed, in the eyes of
> > snd_hdac_bus_process_unsol_events() it is still a valid target to
> > delegate work to. Such behaviour may lead to use-after-free errors.
> > Address by verifying if codec is actually registered.
> > 
> > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> > ---
> >  include/sound/hda_codec.h |  1 -
> >  include/sound/hdaudio.h   |  1 +
> >  sound/hda/hdac_bus.c      |  2 +-
> >  sound/pci/hda/hda_codec.c | 10 +++++-----
> >  sound/soc/codecs/hda.c    |  4 ++--
> 
> This patch doesn't apply cleanly because the change in
> sound/soc/codecs/hda.c still not included in my tree.

Now merged to for-next branch.


thanks,

Takashi

[PATCH 6/9] ALSA: hda: Fix page fault in snd_hda_codec_shutdown()

[Cezary Rojewski]

If early probe of HDAudio bus driver fails e.g.: due to missing
firmware file, snd_hda_codec_shutdown() ends in manipulating
uninitialized codec->pcm_list_head causing page fault.

Iinitialization of HDAudio codec in ASoC is split in two:
- snd_hda_codec_device_init()
- snd_hda_codec_device_new()

snd_hda_codec_device_init() is called during probe_codecs() by HDAudio
bus driver while snd_hda_codec_device_new() is called by
codec-component's ->probe(). The second call will not happen until all
components required by related sound card are present within the ASoC
framework. With firmware failing to load during the PCI's deferred
initialization i.e.: probe_work(), no platform components are ever
registered. HDAudio codec enumeration is done at that point though, so
the codec components became registered to ASoC framework, calling
snd_hda_codec_device_init() in the process.

Now, during platform reboot snd_hda_codec_shutdown() is called for every
codec found on the HDAudio bus causing oops if any of them has not
completed both of their initialization steps. Relocating field
initialization fixes the issue.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/pci/hda/hda_codec.c | 41 +++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 75e85bf58681..677d0a78f19c 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -930,8 +930,28 @@ snd_hda_codec_device_init(struct hda_bus *bus, unsigned int codec_addr,
 	}
 
 	codec->bus = bus;
+	codec->depop_delay = -1;
+	codec->fixup_id = HDA_FIXUP_ID_NOT_SET;
+	codec->core.dev.release = snd_hda_codec_dev_release;
+	codec->core.exec_verb = codec_exec_verb;
 	codec->core.type = HDA_DEV_LEGACY;
 
+	mutex_init(&codec->spdif_mutex);
+	mutex_init(&codec->control_mutex);
+	snd_array_init(&codec->mixers, sizeof(struct hda_nid_item), 32);
+	snd_array_init(&codec->nids, sizeof(struct hda_nid_item), 32);
+	snd_array_init(&codec->init_pins, sizeof(struct hda_pincfg), 16);
+	snd_array_init(&codec->driver_pins, sizeof(struct hda_pincfg), 16);
+	snd_array_init(&codec->cvt_setups, sizeof(struct hda_cvt_setup), 8);
+	snd_array_init(&codec->spdif_out, sizeof(struct hda_spdif_out), 16);
+	snd_array_init(&codec->jacktbl, sizeof(struct hda_jack_tbl), 16);
+	snd_array_init(&codec->verbs, sizeof(struct hda_verb *), 8);
+	INIT_LIST_HEAD(&codec->conn_list);
+	INIT_LIST_HEAD(&codec->pcm_list_head);
+	INIT_DELAYED_WORK(&codec->jackpoll_work, hda_jackpoll_work);
+	refcount_set(&codec->pcm_ref, 1);
+	init_waitqueue_head(&codec->remove_sleep);
+
 	return codec;
 }
 EXPORT_SYMBOL_GPL(snd_hda_codec_device_init);
@@ -984,29 +1004,8 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
 	if (snd_BUG_ON(codec_addr > HDA_MAX_CODEC_ADDRESS))
 		return -EINVAL;
 
-	codec->core.dev.release = snd_hda_codec_dev_release;
-	codec->core.exec_verb = codec_exec_verb;
-
 	codec->card = card;
 	codec->addr = codec_addr;
-	mutex_init(&codec->spdif_mutex);
-	mutex_init(&codec->control_mutex);
-	snd_array_init(&codec->mixers, sizeof(struct hda_nid_item), 32);
-	snd_array_init(&codec->nids, sizeof(struct hda_nid_item), 32);
-	snd_array_init(&codec->init_pins, sizeof(struct hda_pincfg), 16);
-	snd_array_init(&codec->driver_pins, sizeof(struct hda_pincfg), 16);
-	snd_array_init(&codec->cvt_setups, sizeof(struct hda_cvt_setup), 8);
-	snd_array_init(&codec->spdif_out, sizeof(struct hda_spdif_out), 16);
-	snd_array_init(&codec->jacktbl, sizeof(struct hda_jack_tbl), 16);
-	snd_array_init(&codec->verbs, sizeof(struct hda_verb *), 8);
-	INIT_LIST_HEAD(&codec->conn_list);
-	INIT_LIST_HEAD(&codec->pcm_list_head);
-	refcount_set(&codec->pcm_ref, 1);
-	init_waitqueue_head(&codec->remove_sleep);
-
-	INIT_DELAYED_WORK(&codec->jackpoll_work, hda_jackpoll_work);
-	codec->depop_delay = -1;
-	codec->fixup_id = HDA_FIXUP_ID_NOT_SET;
 
 #ifdef CONFIG_PM
 	codec->power_jiffies = jiffies;
-- 
2.25.1

Re: [PATCH 6/9] ALSA: hda: Fix page fault in snd_hda_codec_shutdown()

[Pierre-Louis Bossart]

On 7/6/22 07:02, Cezary Rojewski wrote:
> If early probe of HDAudio bus driver fails e.g.: due to missing
> firmware file, snd_hda_codec_shutdown() ends in manipulating
> uninitialized codec->pcm_list_head causing page fault.
> 
> Iinitialization of HDAudio codec in ASoC is split in two:
> - snd_hda_codec_device_init()
> - snd_hda_codec_device_new()
> 
> snd_hda_codec_device_init() is called during probe_codecs() by HDAudio
> bus driver while snd_hda_codec_device_new() is called by
> codec-component's ->probe(). The second call will not happen until all
> components required by related sound card are present within the ASoC
> framework. With firmware failing to load during the PCI's deferred
> initialization i.e.: probe_work(), no platform components are ever
> registered. HDAudio codec enumeration is done at that point though, so
> the codec components became registered to ASoC framework, calling
> snd_hda_codec_device_init() in the process.
> 
> Now, during platform reboot snd_hda_codec_shutdown() is called for every
> codec found on the HDAudio bus causing oops if any of them has not
> completed both of their initialization steps. Relocating field
> initialization fixes the issue.
> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>

This patch causes an across-the-board regression on all SOF platforms
using an HDaudio or iDISP codec. Only 'nocodec' platforms are
unaffected, see results at
https://sof-ci.01.org/linuxpr/PR3763/build394/devicetest/

Reverting this commit seems to fix the issue.

Upstream merge:
https://github.com/thesofproject/linux/pull/3763

Issue and bisect results
https://github.com/thesofproject/linux/issues/3764

I don't know what this was supposed to fix on the shutdown path but it's
clearly having side effects on the probe/init path.

> ---
>  sound/pci/hda/hda_codec.c | 41 +++++++++++++++++++--------------------
>  1 file changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 75e85bf58681..677d0a78f19c 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -930,8 +930,28 @@ snd_hda_codec_device_init(struct hda_bus *bus, unsigned int codec_addr,
>  	}
>  
>  	codec->bus = bus;
> +	codec->depop_delay = -1;
> +	codec->fixup_id = HDA_FIXUP_ID_NOT_SET;
> +	codec->core.dev.release = snd_hda_codec_dev_release;
> +	codec->core.exec_verb = codec_exec_verb;
>  	codec->core.type = HDA_DEV_LEGACY;
>  
> +	mutex_init(&codec->spdif_mutex);
> +	mutex_init(&codec->control_mutex);
> +	snd_array_init(&codec->mixers, sizeof(struct hda_nid_item), 32);
> +	snd_array_init(&codec->nids, sizeof(struct hda_nid_item), 32);
> +	snd_array_init(&codec->init_pins, sizeof(struct hda_pincfg), 16);
> +	snd_array_init(&codec->driver_pins, sizeof(struct hda_pincfg), 16);
> +	snd_array_init(&codec->cvt_setups, sizeof(struct hda_cvt_setup), 8);
> +	snd_array_init(&codec->spdif_out, sizeof(struct hda_spdif_out), 16);
> +	snd_array_init(&codec->jacktbl, sizeof(struct hda_jack_tbl), 16);
> +	snd_array_init(&codec->verbs, sizeof(struct hda_verb *), 8);
> +	INIT_LIST_HEAD(&codec->conn_list);
> +	INIT_LIST_HEAD(&codec->pcm_list_head);
> +	INIT_DELAYED_WORK(&codec->jackpoll_work, hda_jackpoll_work);
> +	refcount_set(&codec->pcm_ref, 1);
> +	init_waitqueue_head(&codec->remove_sleep);
> +
>  	return codec;
>  }
>  EXPORT_SYMBOL_GPL(snd_hda_codec_device_init);
> @@ -984,29 +1004,8 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
>  	if (snd_BUG_ON(codec_addr > HDA_MAX_CODEC_ADDRESS))
>  		return -EINVAL;
>  
> -	codec->core.dev.release = snd_hda_codec_dev_release;
> -	codec->core.exec_verb = codec_exec_verb;
> -
>  	codec->card = card;
>  	codec->addr = codec_addr;
> -	mutex_init(&codec->spdif_mutex);
> -	mutex_init(&codec->control_mutex);
> -	snd_array_init(&codec->mixers, sizeof(struct hda_nid_item), 32);
> -	snd_array_init(&codec->nids, sizeof(struct hda_nid_item), 32);
> -	snd_array_init(&codec->init_pins, sizeof(struct hda_pincfg), 16);
> -	snd_array_init(&codec->driver_pins, sizeof(struct hda_pincfg), 16);
> -	snd_array_init(&codec->cvt_setups, sizeof(struct hda_cvt_setup), 8);
> -	snd_array_init(&codec->spdif_out, sizeof(struct hda_spdif_out), 16);
> -	snd_array_init(&codec->jacktbl, sizeof(struct hda_jack_tbl), 16);
> -	snd_array_init(&codec->verbs, sizeof(struct hda_verb *), 8);
> -	INIT_LIST_HEAD(&codec->conn_list);
> -	INIT_LIST_HEAD(&codec->pcm_list_head);
> -	refcount_set(&codec->pcm_ref, 1);
> -	init_waitqueue_head(&codec->remove_sleep);
> -
> -	INIT_DELAYED_WORK(&codec->jackpoll_work, hda_jackpoll_work);
> -	codec->depop_delay = -1;
> -	codec->fixup_id = HDA_FIXUP_ID_NOT_SET;
>  
>  #ifdef CONFIG_PM
>  	codec->power_jiffies = jiffies;

Re: [PATCH 6/9] ALSA: hda: Fix page fault in snd_hda_codec_shutdown()

[Takashi Iwai]

On Fri, 15 Jul 2022 20:16:14 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 7/6/22 07:02, Cezary Rojewski wrote:
> > If early probe of HDAudio bus driver fails e.g.: due to missing
> > firmware file, snd_hda_codec_shutdown() ends in manipulating
> > uninitialized codec->pcm_list_head causing page fault.
> > 
> > Iinitialization of HDAudio codec in ASoC is split in two:
> > - snd_hda_codec_device_init()
> > - snd_hda_codec_device_new()
> > 
> > snd_hda_codec_device_init() is called during probe_codecs() by HDAudio
> > bus driver while snd_hda_codec_device_new() is called by
> > codec-component's ->probe(). The second call will not happen until all
> > components required by related sound card are present within the ASoC
> > framework. With firmware failing to load during the PCI's deferred
> > initialization i.e.: probe_work(), no platform components are ever
> > registered. HDAudio codec enumeration is done at that point though, so
> > the codec components became registered to ASoC framework, calling
> > snd_hda_codec_device_init() in the process.
> > 
> > Now, during platform reboot snd_hda_codec_shutdown() is called for every
> > codec found on the HDAudio bus causing oops if any of them has not
> > completed both of their initialization steps. Relocating field
> > initialization fixes the issue.
> > 
> > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> 
> This patch causes an across-the-board regression on all SOF platforms
> using an HDaudio or iDISP codec. Only 'nocodec' platforms are
> unaffected, see results at
> https://sof-ci.01.org/linuxpr/PR3763/build394/devicetest/
> 
> Reverting this commit seems to fix the issue.
> 
> Upstream merge:
> https://github.com/thesofproject/linux/pull/3763
> 
> Issue and bisect results
> https://github.com/thesofproject/linux/issues/3764
> 
> I don't know what this was supposed to fix on the shutdown path but it's
> clearly having side effects on the probe/init path.

Yeah, obviously the patch ignores the fact that hdac_hda does
initialize the HD-audio codec without snd_hda_codec_device_init().

I'm going to revert the commit.


thanks,

Takashi

Re: [PATCH 6/9] ALSA: hda: Fix page fault in snd_hda_codec_shutdown()

[Cezary Rojewski]

On 2022-07-15 8:23 PM, Takashi Iwai wrote:
> On Fri, 15 Jul 2022 20:16:14 +0200,
> Pierre-Louis Bossart wrote:

...

>> This patch causes an across-the-board regression on all SOF platforms
>> using an HDaudio or iDISP codec. Only 'nocodec' platforms are
>> unaffected, see results at
>> https://sof-ci.01.org/linuxpr/PR3763/build394/devicetest/
>>
>> Reverting this commit seems to fix the issue.
>>
>> Upstream merge:
>> https://github.com/thesofproject/linux/pull/3763
>>
>> Issue and bisect results
>> https://github.com/thesofproject/linux/issues/3764
>>
>> I don't know what this was supposed to fix on the shutdown path but it's
>> clearly having side effects on the probe/init path.
> 
> Yeah, obviously the patch ignores the fact that hdac_hda does
> initialize the HD-audio codec without snd_hda_codec_device_init().
> 
> I'm going to revert the commit.


Thanks for the report and sorry the trouble.

This patch is still valid - will re-check hdac_hda.c and update it so 
that both the regression and the page fault are gone.


Regards,
Czarek

[PATCH 7/9] ALSA: hda: Reset all SIE bits in INTCTL

[Cezary Rojewski]

From: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>

Old code resets SIE for up to 8 streams using byte accessor, but
register is laid out in following way:

31 GIE
30 CIE
29:x Reserved
x-1:0 SIE

If there is more than 8 streams, some of them may and up with enabled
interrupts. To fix this just clear whole INTCTL register when disabling
interrupts.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/hda/hdac_controller.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c
index f7bd6e2db085..9a60bfdb39ba 100644
--- a/sound/hda/hdac_controller.c
+++ b/sound/hda/hdac_controller.c
@@ -474,11 +474,8 @@ static void azx_int_disable(struct hdac_bus *bus)
 	list_for_each_entry(azx_dev, &bus->stream_list, list)
 		snd_hdac_stream_updateb(azx_dev, SD_CTL, SD_INT_MASK, 0);
 
-	/* disable SIE for all streams */
-	snd_hdac_chip_writeb(bus, INTCTL, 0);
-
-	/* disable controller CIE and GIE */
-	snd_hdac_chip_updatel(bus, INTCTL, AZX_INT_CTRL_EN | AZX_INT_GLOBAL_EN, 0);
+	/* disable SIE for all streams & disable controller CIE and GIE */
+	snd_hdac_chip_writel(bus, INTCTL, 0);
 }
 
 /* clear interrupts */
-- 
2.25.1

[PATCH 8/9] ALSA: hda: Remove unused macro definition

[Cezary Rojewski]

From: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>

It is not used anywhere in the file, so there is no need to keep it.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/hda/ext/hdac_ext_controller.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/sound/hda/ext/hdac_ext_controller.c b/sound/hda/ext/hdac_ext_controller.c
index b072392725c7..a42f66f561f5 100644
--- a/sound/hda/ext/hdac_ext_controller.c
+++ b/sound/hda/ext/hdac_ext_controller.c
@@ -14,13 +14,6 @@
 #include <sound/hda_register.h>
 #include <sound/hdaudio_ext.h>
 
-/*
- * maximum HDAC capablities we should parse to avoid endless looping:
- * currently we have 4 extended caps, so this is future proof for now.
- * extend when this limit is seen meeting in real HW
- */
-#define HDAC_MAX_CAPS 10
-
 /*
  * processing pipe helpers - these helpers are useful for dealing with HDA
  * new capability of processing pipelines
-- 
2.25.1

[PATCH 9/9] ALSA: hda/realtek: Remove redundant init_hook() in alc_default_init()

[Cezary Rojewski]

snd_hda_gen_init() does that for every codec already.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/pci/hda/patch_realtek.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index f3ad454b3fbf..a8688025352d 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -923,9 +923,6 @@ static int alc_init(struct hda_codec *codec)
 	if (is_s4_resume(codec))
 		alc_pre_init(codec);
 
-	if (spec->init_hook)
-		spec->init_hook(codec);
-
 	spec->gen.skip_verbs = 1; /* applied in below */
 	snd_hda_gen_init(codec);
 	alc_fix_pll(codec);
-- 
2.25.1

Re: [PATCH 9/9] ALSA: hda/realtek: Remove redundant init_hook() in alc_default_init()

[Takashi Iwai]

On Wed, 06 Jul 2022 14:02:30 +0200,
Cezary Rojewski wrote:
> 
> snd_hda_gen_init() does that for every codec already.

Definitely not.

snd_hda_gen_init() calls init_hook of snd_hda_gen_spec.

OTOH, what you're trying to kill the init_hook call of alc_spec.
Both are in different layers and they don't share the same callback.


thanks,

Takashi

> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  sound/pci/hda/patch_realtek.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> index f3ad454b3fbf..a8688025352d 100644
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -923,9 +923,6 @@ static int alc_init(struct hda_codec *codec)
>  	if (is_s4_resume(codec))
>  		alc_pre_init(codec);
>  
> -	if (spec->init_hook)
> -		spec->init_hook(codec);
> -
>  	spec->gen.skip_verbs = 1; /* applied in below */
>  	snd_hda_gen_init(codec);
>  	alc_fix_pll(codec);
> -- 
> 2.25.1
> 

Re: [PATCH 9/9] ALSA: hda/realtek: Remove redundant init_hook() in alc_default_init()

[Cezary Rojewski]

On 2022-07-09 6:46 PM, Takashi Iwai wrote:
> On Wed, 06 Jul 2022 14:02:30 +0200,
> Cezary Rojewski wrote:
>>
>> snd_hda_gen_init() does that for every codec already.
> 
> Definitely not.
> 
> snd_hda_gen_init() calls init_hook of snd_hda_gen_spec.
> 
> OTOH, what you're trying to kill the init_hook call of alc_spec.
> Both are in different layers and they don't share the same callback.


I see the mistake now. This patch was generated when I was debugging one 
of the module-reload issues, and once it was fixed, double ->init_hook() 
caught my eye so I decided to add additional cleanup patch on top. Meh.

Thanks for paying attention and good review!


Regards,
Czarek

Re: [PATCH 0/9] ALSA: hda: Codec-reload bug fixes and cleanups

[Takashi Iwai]

On Wed, 06 Jul 2022 14:02:21 +0200,
Cezary Rojewski wrote:
> 
> Total of 6 fixes and 3 cleanups - cleanups are last.
> 
> All of the fixes address problems that present themselves in situation
> when user engages in codec driver reload. Second condition to reproduce
> is two-step initialization of HDAudio codec - this is the case only for
> ASoC HDAudio bus driver as snd_hda_intel calls only compound function
> snd_hda_codec_new(). Once these conditions are met, several
> reload/unload scenarios end with null-ptr-deref and page faults. Goal of
> the series is to allow codec/bus driver reloading without any errors.
> 
> Amadeusz Sławiński (2):
>   ALSA: hda: Reset all SIE bits in INTCTL
>   ALSA: hda: Remove unused macro definition
> 
> Cezary Rojewski (7):
>   ALSA: hda: Do not unset preset when cleaning up codec
>   ALSA: hda: Fix null-ptr-deref when i915 fails and hdmi is denylisted
>   ALSA: hda: Make device usage_count consistent across subsequent
>     probing
>   ALSA: hda: Fix put_device() inconsistency in error path
>   ALSA: hda: Skip event processing for unregistered codecs
>   ALSA: hda: Fix page fault in snd_hda_codec_shutdown()
>   ALSA: hda/realtek: Remove redundant init_hook() in alc_default_init()

Now applied partially what are applicable and look correct: patches
2, 3, 4, 6, 7, and 8.

Patch 5 waits for Mark's PR of ASoC changes, and patch 1 needs a bit
more clarification and investigation.


thanks,

Takashi