"Kernel Panic" run 'soft/kmod_scripts/sof_bootone.sh' cause kernel panic on BYT with sof-byt-rt5651.tplg

[ghost]

System Environment:
Kernel repo: topic/sof-dev: 46b7b64
soft repo: topic/master: cfff9e0
using the tplg: sof-byt-rt5651.tplg

Reproduce step:
run the "soft/kmod_scripts/sof_bootone.sh" script, that means do the module reload one time.
the panic will be hit.

the log:
dmesg.txt

[ghost]

the root cause is found:
It is related to the rt5651's tplg. the rt5651 use the "get/put/info ID" with 259.
but the kernel has not merged this patch accordingly.
which will cause the "Tone Switch 5" kcontol allocate failure.
when we do the module reload, this will cause the kernel panic in the remove_widget() function.
because the null pointer is trying to be accessed in this function.

[plbossart]

Are you saying the kernel oops happens in the chunk of code below?
If yes, can you try and protect the dereferences when kcontrols is NULL?

if (dobj->widget.kcontrol_type == SND_SOC_TPLG_TYPE_ENUM) {
/* enumerated widget mixer */
for (i = 0; w->kcontrols != NULL && i < w->num_kcontrols; i++) {
struct snd_kcontrol *kcontrol = w->kcontrols[i];
struct soc_enum *se =
(struct soc_enum *)kcontrol->private_value;
int j;

		snd_ctl_remove(card, kcontrol);

		kfree(se->dobj.control.dvalues);
		for (j = 0; j < se->items; j++)
			kfree(se->dobj.control.dtexts[j]);

		kfree(se);
		kfree(w->kcontrol_news[i].name);
	}
} else {
	/* volume mixer or bytes controls */
	for (i = 0; w->kcontrols != NULL && i < w->num_kcontrols; i++) {
		struct snd_kcontrol *kcontrol = w->kcontrols[i];

		if (dobj->widget.kcontrol_type
		    == SND_SOC_TPLG_TYPE_MIXER)
			kfree(kcontrol->tlv.p);

		/* Private value is used as struct soc_mixer_control
		 * for volume mixers or soc_bytes_ext for bytes
		 * controls.
		 */
		kfree((void *)kcontrol->private_value);
		snd_ctl_remove(card, kcontrol);
		kfree(w->kcontrol_news[i].name);
	}
}

[ghost]

@plbossart
you are exactly right.
in this panic case, it is caused by below. the kcontrol is actually NULL already, access the pointer "kcontrol->tlv.p" will cause panic.
if (dobj->widget.kcontrol_type
== SND_SOC_TPLG_TYPE_MIXER)
kfree(kcontrol->tlv.p);

I will make a PR for this protection.

[plbossart]

Thanks. Since this is going to touch non-SOF code in soc-topology, we'll have to be more generic and check what happens for other widgets as well, not just the control ones.
Alternatively we'd want to bail when the unknown binding is found, that might actually be a better way of doing things?

[ghost]

@plbossart
Yes, I agree with you. we can add check here, this could be the last line.
and add more code to do more check in the kcontrol allocate location.
I will do more debug for this issue.
thank you!

[ghost]

@plbossart @lgirdwood
I found two questions during this debugging:

1. I did not find location to free the instance of "struct snd_sof_control{}".
   we allocate this instance in sof_control_load(), this function's call-stack is below:
   soc_tplg_dapm_widget_dmixer_create()
   soc_tplg_init_kcontrol()
   tplg->ops->control_load()
   sof_control_load()

but I did not find the free location, normally I think it should be in sof_control_unload() function.
but actually it is not there. it might be a memory leak.
its call-stack is shown below:
snd_soc_tplg_component_remove()
remove_mixer()
dobj->ops->control_unload()
sof_control_unload()

2. I have to update the description of the root cause of this panic issue again(maybe I am wrong, please point it out)
   when the soc_tplg_dapm_widget_dmixer_create() is called to construct the instance of kcontrol.
   actually, it will construct the instance "struct snd_sof_control{}", not to construct the instance "struct snd_kcontrol{}". so it will has problem when we try to free the instance in remove_widget() function.
   because the snd_kcontrol{} is never allocated, only the snd_sof_kcontrol{} is allocated.

please revert to me, if I am wrong.
thank you!

[ghost]

the remove_widget() did not call the sof_control_unload() function. so it has no chance to free the snd_sof_control{} instance.

[ghost]

Just now I tried: call the control_unload() in remove_widget(). it is not right. also will has the panic.
I just wonder where is the location to free the snd_sof_control{} instance.

[lgirdwood]

@zhigang-wu please try and switch on the kernel memory debugging tools (under kconfig debug options). This will mean a kernel rebuild, but it does help to shed light on any double frees or use after free issues.

[ghost]

@lgirdwood
I will try it. thank you for your suggestion.

[ghost]

this issue can be fixed by PR #72

[ghost]

the PR #80 and PR #72 can fix this kind of kernel panic issue already.

[ZhendanYang]

This issue still exists on latest code base:
sof-dev : 108d9cf
sof tool: 78748d4
sof stable-1.2: 7dd4b1d

[ZhendanYang]

This issue can still reproduce with latest code base:
kernel: sof-dev 160b45f
sof tool: 57b5212
sof stable-1.2 : 7dd4b1d
dmesg-panic.log

[mengdonglin]

@stevyan @markyang Could you help to check if this issue is gone with fixing of #144 by #311?

[markyang]

Summary:
This issue cannot be reproduced on BYT with ALC5651 using the build on Dec 7
There is no Kernel Panic, both arecord and aplay worked fine.

Test steps:

1. kmod_scripts/sof_bootone.sh
2. arecord -Dhw:0,0 -c2 -fS16_LE -vv -i ~/mytest.wav #OK
3. aplay -Dhw:0,0 -c2 -fS16_LE -vv -i ~/mytest.wav #OK

Test env:
sof master: b5d6c71 #Dec 7
kernel sof-dev: a71221d #Dec 7
tplg: test-ssp2-mclk-0-I2S-volume-s16le-s16le-48k-19200k-codec.tplg