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

[Takashi Iwai <tiwai@suse.de>]

On Thu, 26 Nov 2015 12:24:58 +0100,
Vinod Koul wrote:
> 
> On Thu, Nov 26, 2015 at 10:19:51AM +0100, Takashi Iwai wrote:
> > On Thu, 26 Nov 2015 10:10:16 +0100,
> > Vinod Koul wrote:
> > > 
> > > On Thu, Nov 26, 2015 at 09:48:47AM +0100, Takashi Iwai wrote:
> > > > On Thu, 26 Nov 2015 15:11:00 +0100,
> > > > Subhransu S. Prusty wrote:
> > > > > 
> > > > > During element creation, the name of some of the elements point
> > > > > to memory referenced in tplg fw. If the tplg fw is released after
> > > > > tplg is parsed by framework, kernel panic happens during creation
> > > > > of elements while card initialization.
> > > > 
> > > > In which code path?  When the kctl is already instantiated from
> > > > snd_kcontrol_new template, we don't have to duplicate the string.
> > > > The only case where the strdup() is required is to delay the
> > > > instantiation, i.e. storing the kcontrol_new object itself instead of
> > > > referring temporarily.
> > > 
> > > So in SKL, we do request firmware of topology binary and topology core uses
> > > that for strings here, so the patch 87b5ed8ec freed the topology binary
> > > which causes panic while accessing kcontrols.
> > 
> > This is strange.  If it's about the kctl name string, the panic
> > shouldn't happen at accessing the kctl but at instantiating the kctl
> > from snd_kcontrol_new that contains the invalid string pointer.
> > The kctl object contains the string in itself, and there copies the
> > string from the template.
> > 
> > Also I wonder why it kernel panics, not the normal Oops.
> 
> Sorry it a oops, paging request failure and not a panic
> 
> > > Your second point is applicable here as card instantiation is delayed often
> > > for us as all components may not be present and delayed probe finally
> > > creates the card.
> > > 
> > > > > Issue is caught with id#87b5ed8ecb9fe05a696e1c0b53c7a49ea66432c1
> > > > 
> > > > You should put the commit subject, too.
> > > 
> > > Yes we will add that
> > > 
> > > > > So create a copy of the memory and assign to names instead.
> > > > 
> > > > And who releases these duplicated memory?  It looks like another
> > > > memory leak to me.
> > > 
> > > That is a good point and I think we should do devm_kstrdup() here so that
> > > this is freed when we cleanup the device, or do you have any better
> > > suggestion ?
> > 
> > devm_kstrdup() is bad in this case.  You can reload the topology
> > unlimitedly, and the memory won't be freed until the device unbind,
> > thus it keeps hogging.
> > 
> > You really need to identify which path hits the issue exactly how.  In
> > general, the string passed to template is only for creating the kctl.
> > Once when kctl is created, the whole snd_kcontrol_new template and the
> > allocated string is no use, so they can be freed.
> 
> but then question of where should these be freed. For current drivers they
> declare controls statically, so memory is always there.. How do free up in
> the cases where we allocate dynamically?

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.


Takashi