Re: [PATCH] ASoC: topology: Fix not to keep a reference to tplg fw

[Takashi Iwai <tiwai@suse.de>]

On Fri, 27 Nov 2015 12:42:04 +0100,
Subhransu S. Prusty wrote:
> 
> 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.

Then it's a driver's failure.  It needs to call it appropriately
before reloading.  Or it was supposed to be invoked from
snd_soc_tplg_component_remove()?  I don't know.


Takashi