[alsa-devel] [PATCH] ASoC: dapm: fix module unload/reload failed issue by reset kcontrol's widget list

[Lars-Peter Clausen]

On 07/28/2015 04:34 PM, Jie, Yang wrote:
>> -----Original Message-----
>> From: Takashi Iwai [mailto:tiwai at suse.de]
>> Sent: Tuesday, July 28, 2015 5:07 PM
>> To: Jie, Yang
>> Cc: broonie at kernel.org; alsa-devel at alsa-project.org; Girdwood, Liam R
>> Subject: Re: [PATCH] ASoC: dapm: fix module unload/reload failed issue by
>> reset kcontrol's widget list
>>
>> On Tue, 28 Jul 2015 11:03:39 +0200,
>> Jie Yang wrote:
>>>
>>> We need reset the related kcontrol's widget list during the freeing of
>>> a widget, otherwise, the widget list may be outdated and reference to
>>> it may introduce errors(they usually occurs during driver modules
>>> unloading/reloading).
>>>
>>> Here adding a func dapm_update_kcontrols_of_freeing_widget and call it
>>> at dapm_update_kcontrols_of_freeing_widget, to fix those issues.
>>>
>>> Signed-off-by: Jie Yang <yang.jie at intel.com>
>>> ---
>>>   sound/soc/soc-dapm.c | 31 +++++++++++++++++++++++++++++++
>>>   1 file changed, 31 insertions(+)
>>>
>>> diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index
>>> aa327c9..1ad8144 100644
>>> --- a/sound/soc/soc-dapm.c
>>> +++ b/sound/soc/soc-dapm.c
>>> @@ -2305,6 +2305,35 @@ static void dapm_free_path(struct
>> snd_soc_dapm_path *path)
>>>   	kfree(path);
>>>   }
>>>
>>> +/* update all dapm kcontrols that related to a widget which being
>>> +freed*/ static void dapm_update_kcontrols_of_freeing_widget(struct
>>> +snd_soc_dapm_widget * w) {
>>> +	struct dapm_kcontrol_data *data = NULL;
>>> +	int i, j, n;
>>> +
>>> +	if (w && w->kcontrols) {
>>> +		for (i = 0; i < w->num_kcontrols; i++) {
>>> +			if (w->kcontrols[i] == NULL)
>>> +				continue;
>>> +			data = snd_kcontrol_chip(w->kcontrols[i]);
>>> +			if (data == NULL)
>>> +				continue;
>>> +			for (j = 0, n = 0; j < data->wlist->num_widgets; j++) {
>>> +				if (data->wlist->widgets[j] == w) {
>>> +					data->wlist->widgets[j] = NULL;
>>> +					n++;
>>> +				}
>>> +			}
>>> +
>>> +			if (n) {
>>> +				data->wlist->num_widgets -= n;
>>
>> This looks buggy.  You didn't rearrange widgets[] but just put a hole, yet
>> decreasing the number.
>
> You are right, but since krealloc() cannot shrink the allocated size, maybe we should
> free those related kcontrols here?
>
>>
>> BTW, how does the reference occur while unloading?  Or better to say:
>> isn't it the module refcount adjusted while accessing the resources?
>
>
> soc_cleanup_card_resources
> 	snd_soc_dapm_free
> 		dapm_free_widgets
> 	snd_card_free
> 		//removing kcontrols, calling private dapm_kcontrol_free()
> 			kfree(data->widget->name);//the widget here may be
> 					//already freed! So the reference here
> 					//may cause error.

Hi,

that free was wrong no matter what, since the widget itself frees the name. 
So even if widget is valid this would result in a double free.

The issue was fixed in this commit: 
http://git.kernel.org/cgit/linux/kernel/git/broonie/sound.git/commit?h=for-next&id=e18077b6e5dfe26e9fbbdc1fd1085a1701c24bea

- Lars