Skip to content

ASoC:topology: bug fix oops cause by pointer dereference. #72

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ghost wants to merge 1 commit into from
Closed

ASoC:topology: bug fix oops cause by pointer dereference. #72

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions sound/soc/soc-dapm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3046,6 +3046,7 @@ int snd_soc_dapm_new_widgets(struct snd_soc_card *card)
{
struct snd_soc_dapm_widget *w;
unsigned int val;
int ret = 0;

mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_INIT);

Expand All @@ -3068,23 +3069,30 @@ int snd_soc_dapm_new_widgets(struct snd_soc_card *card)
case snd_soc_dapm_switch:
case snd_soc_dapm_mixer:
case snd_soc_dapm_mixer_named_ctl:
dapm_new_mixer(w);
ret = dapm_new_mixer(w);
Copy link
Member

@lgirdwood lgirdwood Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking the return is good, but dapm_new_mixer, dapm_new_mux etc should free any resources they allocate on failure. likewise dapm_create_or_share_kcontrol() will also need to be check that it frees any resource it allocates on failure. So please

  1. Check return values and propagate the return values up the call stack.
  2. Free any resources in the function that allocates them on any failure.

Copy link
Author

@ghost ghost Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I checked the code in the dapm_create_or_share_kcontrol(), when it detected the error and before return, it has already finished free operation. in this functin, it has only two allocation operation for long_name and kcontrol.
    like the snd_soc_dapm_add_path(), it also do this job already when return with error.
  2. we only care about the w->kcontrols itself. when the sub-function return error. it will free the allocated w->kcontrols.
  3. But the updated PR also has the problem: when w->num_kcontrols is not equal to 0. but the code goto the default case. it will cause the w->kcontrols will be allocated. but never be processed further.
    I think we have to free it when enter the default case.

break;
case snd_soc_dapm_mux:
case snd_soc_dapm_demux:
dapm_new_mux(w);
ret = dapm_new_mux(w);
break;
case snd_soc_dapm_pga:
case snd_soc_dapm_out_drv:
dapm_new_pga(w);
ret = dapm_new_pga(w);
break;
case snd_soc_dapm_dai_link:
dapm_new_dai_link(w);
ret = dapm_new_dai_link(w);
break;
default:
break;
Copy link
Author

@ghost ghost Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should the code in the default case:
if (w->num_kcontrols) {
kfree(w->kcontrols);
w->kcontrols = NULL;
}

Copy link
Member

@lgirdwood lgirdwood Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, do it outside default case otherwise we wont kfree() all resources.

Copy link
Author

@ghost ghost Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgirdwood
what should i do if we are in this case?
the w->num_kcontrols != 0, the default case will be entered.

in this case, the w->kcontrols will be allocated. but it is not processed by sub-function, because the default case is entered. then question comes: should we kfree this allocated w->kcontrols at this time?

If yes, we have to kfree it. then I will roll back to the previous version: adding the flag in the default case. when the flag is set in the default. we will do the kfree.

if no, I think we can not cover the "siggen" case, which cause our panic when in tplg free stage.
its w->num_kcontrols = 1, but "snd_soc_dapm_siggen" is not in this switch{}, it will enter the default case.

Copy link
Member

@lgirdwood lgirdwood Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhigang-wu I need you to improve the current patch to consider all error paths in the function. default case is a valid for other widget types so cant be used as a means of freeing resources. Just walk through this function line by line and see where things could fail and then ask where do I recover this ?

Copy link
Collaborator

@lyakh lyakh Sep 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgirdwood that function adds new widgets by walking a linked list of them. If adding one of them fails, looks like previously successfully added widgets are kept. Without this patch in such a case the function could return an error, if allocation failed, or 0, if initialisation failed. I can see the following possibilities in case of a partial success:

  1. free all so far successfully added widgets, using snd_soc_dapm_free_widget() and return an error
  2. keep all successfully added widgets and return 0
  3. keep and return an error seems illogical to me
    Which one would you prefer? 1 seems the most consistent to me. Also note, that most users of the function don't even check its return code...

}

if (ret < 0) {
kfree(w->kcontrols);
w->kcontrols = NULL;
mutex_unlock(&card->dapm_mutex);
return ret;
}

/* Read the initial power state from the device */
if (w->reg >= 0) {
soc_dapm_read(w->dapm, w->reg, &val);
Expand Down