[alsa-devel] [PATCH] ASoC: topology: Fix not to keep a reference to tplg fw

[Subhransu S. Prusty]

On Fri, Nov 27, 2015 at 06:54:15AM +0100, Takashi Iwai wrote:
> On Fri, 27 Nov 2015 10:15:19 +0100,
> Subhransu S. Prusty wrote:
> > 
> > On Thu, Nov 26, 2015 at 06:39:02PM +0100, Takashi Iwai wrote:
> > > On Thu, 26 Nov 2015 17:13:43 +0100,
> > > Vinod Koul wrote:
> > > > 
> > > > On Thu, Nov 26, 2015 at 12:46:24PM +0100, Takashi Iwai wrote:
> > > > 
> > > > > 
> > > > > Well, for judging this, we have to follow the code more closely.  And
> > > > > it's why I asked which path does it happen exactly.
> > > > > 
> > > > > There are two different paths where the snd_kcontrol_new is used: the
> > > > > standard controls and dapm.  The former is immediately instantiated
> > > > > via snd_soc_cnew(), so it's fine as is, no need to change.  But the
> > > > > latter is different.
> > > > > 
> > > > > The latter, dapm case, always allocates the snd_kcontrol_new array in
> > > > > kcontrol_news field.  So, we need to change in each function
> > > > > allocating this to do kstrdump() for each kcontrol_new element, and
> > > > > each place calling kfree() of kcontrol_news should free the string of
> > > > > each item in return.
> > > > 
> > > > It is the latter dapm case with added complexity of topology core creating
> > > > these kcontrols. I will reproduce this and send the oops tomorrow
> > > 
> > > Not too complex in this case because there are only a few users.
> > > A totally untested patch is below.
> > > 
> > > 
> > > Takashi
> > > 
> > > ---
> > > diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> > > index 8d7ec80af51b..1f684975b541 100644
> > > --- a/sound/soc/soc-topology.c
> > > +++ b/sound/soc/soc-topology.c
> > > @@ -427,6 +427,16 @@ static void remove_enum(struct snd_soc_component *comp,
> > >  	kfree(se);
> > >  }
> > >  
> > > +static void free_kcontrol_news(const struct snd_kcontrol_new *_wc, int nums)
> > > +{
> > > +	struct snd_kcontrol_new *wc = (struct snd_kcontrol_new *)_wc;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < nums && wc[i].name; i++)
> > > +		kfree(wc[i].name);
> > > +	kfree(wc);
> > > +}
> > > +
> > >  /* remove a byte kcontrol */
> > >  static void remove_bytes(struct snd_soc_component *comp,
> > >  	struct snd_soc_dobj *dobj, int pass)
> > > @@ -477,7 +487,7 @@ static void remove_widget(struct snd_soc_component *comp,
> > >  			kfree(se->dobj.control.dtexts[i]);
> > >  
> > >  		kfree(se);
> > > -		kfree(w->kcontrol_news);
> > > +		free_kcontrol_news(w->kcontrol_news, 1);
> > >  	} else {
> > >  		/* non enumerated widget mixer */
> > >  		for (i = 0; i < w->num_kcontrols; i++) {
> > > @@ -490,7 +500,7 @@ static void remove_widget(struct snd_soc_component *comp,
> > >  			snd_ctl_remove(card, w->kcontrols[i]);
> > >  			kfree(sm);
> > >  		}
> > > -		kfree(w->kcontrol_news);
> > > +		free_kcontrol_news(w->kcontrol_news, w->num_kcontrols);
> > Hi Takashi,
> > 
> > I have not tested this patch yet. But it should fix the oops. Just looking
> > the code I find remove_widget is either called from snd_soc_tplg_widget_remove
> > or from snd_soc_tplg_component_remove. The xxx_component_remove is called
> > during unregister of the component and there is no caller to
> > snd_soc_tplg_widget_remove.
> > 
> > I guess the intention here is to free the kcontrol_news immediately after the
> > card is registered. Please correct me if I am wrong.
> 
> It is already freed in the original code.  The only addition is to
> free the newly allocated strings in kcontrol_news.  So kfree() is
> replaced with free_kcontrol_news().
> 
> > Otherwise shouldn't the devm version of kstrdup work good as it just frees
> > the memory when the device is removed?
> 
> No, as already mentioned, devm won't release the data until unbind and
> the topology data might be reloaded repeatedly, thus user can hog the
> kernel memory unlimitedly.
Then which of the APIs snd_soc_tplg_widget_remove or
snd_soc_tplg_component_remove should free the memory in this scenairo? I
guess it should be snd_soc_tplg_widget_remove, but I don't see a caller of
this API.

Regards,
Subhransu
> 
> 
> Takashi
> 
> > 
> > -- 
> > 

--