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

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

[Takashi Iwai]

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.

> Issue is caught with id#87b5ed8ecb9fe05a696e1c0b53c7a49ea66432c1

You should put the commit subject, too.

> 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.


Takashi

> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
>  sound/soc/soc-topology.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> index 6963ba2..61eb1de 100644
> --- a/sound/soc/soc-topology.c
> +++ b/sound/soc/soc-topology.c
> @@ -709,7 +709,7 @@ static int soc_tplg_dbytes_create(struct soc_tplg *tplg, unsigned int count,
>  			be->hdr.name, be->hdr.access);
>  
>  		memset(&kc, 0, sizeof(kc));
> -		kc.name = be->hdr.name;
> +		kc.name = kstrdup(be->hdr.name, GFP_KERNEL);
>  		kc.private_value = (long)sbe;
>  		kc.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
>  		kc.access = be->hdr.access;
> @@ -789,7 +789,7 @@ static int soc_tplg_dmixer_create(struct soc_tplg *tplg, unsigned int count,
>  			mc->hdr.name, mc->hdr.access);
>  
>  		memset(&kc, 0, sizeof(kc));
> -		kc.name = mc->hdr.name;
> +		kc.name = kstrdup(mc->hdr.name, GFP_KERNEL);
>  		kc.private_value = (long)sm;
>  		kc.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
>  		kc.access = mc->hdr.access;
> @@ -935,7 +935,7 @@ static int soc_tplg_denum_create(struct soc_tplg *tplg, unsigned int count,
>  			ec->hdr.name, ec->items);
>  
>  		memset(&kc, 0, sizeof(kc));
> -		kc.name = ec->hdr.name;
> +		kc.name = kstrdup(ec->hdr.name, GFP_KERNEL);
>  		kc.private_value = (long)se;
>  		kc.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
>  		kc.access = ec->hdr.access;
> @@ -1105,8 +1105,8 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
>  			SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
>  			return -EINVAL;
>  
> -		route.source = elem->source;
> -		route.sink = elem->sink;
> +		route.source = kstrdup(elem->source, GFP_KERNEL);
> +		route.sink = kstrdup(elem->sink, GFP_KERNEL);
>  		route.connected = NULL; /* set to NULL atm for tplg users */
>  		if (strnlen(elem->control, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) == 0)
>  			route.control = NULL;
> @@ -1149,7 +1149,7 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_dmixer_create(
>  		dev_dbg(tplg->dev, " adding DAPM widget mixer control %s at %d\n",
>  			mc->hdr.name, i);
>  
> -		kc[i].name = mc->hdr.name;
> +		kc[i].name = kstrdup(mc->hdr.name, GFP_KERNEL);
>  		kc[i].private_value = (long)sm;
>  		kc[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
>  		kc[i].access = mc->hdr.access;
> @@ -1228,7 +1228,7 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_denum_create(
>  	dev_dbg(tplg->dev, " adding DAPM widget enum control %s\n",
>  		ec->hdr.name);
>  
> -	kc->name = ec->hdr.name;
> +	kc->name = kstrdup(ec->hdr.name, GFP_KERNEL);
>  	kc->private_value = (long)se;
>  	kc->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
>  	kc->access = ec->hdr.access;
> @@ -1330,7 +1330,7 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_dbytes_create(
>  			"ASoC: adding bytes kcontrol %s with access 0x%x\n",
>  			be->hdr.name, be->hdr.access);
>  
> -		kc[i].name = be->hdr.name;
> +		kc[i].name = kstrdup(be->hdr.name, GFP_KERNEL);
>  		kc[i].private_value = (long)sbe;
>  		kc[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
>  		kc[i].access = be->hdr.access;
> -- 
> 1.9.1
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

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

[Vinod Koul]

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.

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 ?

-- 
~Vinod

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

[Takashi Iwai]

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.

> 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.


Takashi

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1226 bytes --]

On Thu, Nov 26, 2015 at 10:19:51AM +0100, Takashi Iwai wrote:
> Vinod Koul wrote:

> > 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.

I guess it's possible that if the control creation happens soon enough
after the memory is freed the data will still be valid.  This could be
tested for by hacking things to deliberately trash the memory before we
get to control creation.

> 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.

That does suggest a fairly simple fix of just holding on to the firmware
for longer assuming that the analysis is correct.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 237 bytes --]

On Thu, Nov 26, 2015 at 07:41:00PM +0530, Subhransu S. Prusty wrote:

> Issue is caught with id#87b5ed8ecb9fe05a696e1c0b53c7a49ea66432c1

Please provide human readable descriptions of things - the id# means
that's not even pasteable. :(

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Takashi Iwai]

On Thu, 26 Nov 2015 12:01:23 +0100,
Mark Brown wrote:
> 
> On Thu, Nov 26, 2015 at 10:19:51AM +0100, Takashi Iwai wrote:
> > Vinod Koul wrote:
> 
> > > 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.
> 
> I guess it's possible that if the control creation happens soon enough
> after the memory is freed the data will still be valid.  This could be
> tested for by hacking things to deliberately trash the memory before we
> get to control creation.
> 
> > 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.
> 
> That does suggest a fairly simple fix of just holding on to the firmware
> for longer assuming that the analysis is correct.

Right, that would be the simplest fix.  Just assure that the whole f/w
image is kept until all objects are instantiated.


Takashi

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

[Vinod Koul]

[-- Attachment #1.1: Type: text/plain, Size: 416 bytes --]

On Thu, Nov 26, 2015 at 11:02:54AM +0000, Mark Brown wrote:
> On Thu, Nov 26, 2015 at 07:41:00PM +0530, Subhransu S. Prusty wrote:
> 
> > Issue is caught with id#87b5ed8ecb9fe05a696e1c0b53c7a49ea66432c1
> 
> Please provide human readable descriptions of things - the id# means
> that's not even pasteable. :(

Sorry about that, I did ask Subhransu to send out rev 2 with proper style

Thanks
-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Vinod Koul]

On Thu, Nov 26, 2015 at 12:03:56PM +0100, Takashi Iwai wrote:
> On Thu, 26 Nov 2015 12:01:23 +0100,
> Mark Brown wrote:
> > 
> > On Thu, Nov 26, 2015 at 10:19:51AM +0100, Takashi Iwai wrote:
> > > Vinod Koul wrote:
> > 
> > > > 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.
> > 
> > I guess it's possible that if the control creation happens soon enough
> > after the memory is freed the data will still be valid.  This could be
> > tested for by hacking things to deliberately trash the memory before we
> > get to control creation.
> > 
> > > 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.
> > 
> > That does suggest a fairly simple fix of just holding on to the firmware
> > for longer assuming that the analysis is correct.
> 
> Right, that would be the simplest fix.  Just assure that the whole f/w
> image is kept until all objects are instantiated.

Yes, going by the discussion here, we can then free the topology firmware
later, but then question is how do we know when is the card completely
instantiated and we can free the topology binary... I do not know how..

Only thing I can think of is to free this is driver .remove()

So then we can simply revert 87b5ed8ecb : ('ASoC: Intel: Skylake: fix memory
leak) and then add a new one..

Thanks
-- 
~Vinod

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

[Vinod Koul]

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?

-- 
~Vinod

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

[Takashi Iwai]

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

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

[Subhransu S. Prusty]

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.

Issue is caught with id#87b5ed8ecb9fe05a696e1c0b53c7a49ea66432c1

So create a copy of the memory and assign to names instead.

Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/soc-topology.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 6963ba2..61eb1de 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -709,7 +709,7 @@ static int soc_tplg_dbytes_create(struct soc_tplg *tplg, unsigned int count,
 			be->hdr.name, be->hdr.access);
 
 		memset(&kc, 0, sizeof(kc));
-		kc.name = be->hdr.name;
+		kc.name = kstrdup(be->hdr.name, GFP_KERNEL);
 		kc.private_value = (long)sbe;
 		kc.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
 		kc.access = be->hdr.access;
@@ -789,7 +789,7 @@ static int soc_tplg_dmixer_create(struct soc_tplg *tplg, unsigned int count,
 			mc->hdr.name, mc->hdr.access);
 
 		memset(&kc, 0, sizeof(kc));
-		kc.name = mc->hdr.name;
+		kc.name = kstrdup(mc->hdr.name, GFP_KERNEL);
 		kc.private_value = (long)sm;
 		kc.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
 		kc.access = mc->hdr.access;
@@ -935,7 +935,7 @@ static int soc_tplg_denum_create(struct soc_tplg *tplg, unsigned int count,
 			ec->hdr.name, ec->items);
 
 		memset(&kc, 0, sizeof(kc));
-		kc.name = ec->hdr.name;
+		kc.name = kstrdup(ec->hdr.name, GFP_KERNEL);
 		kc.private_value = (long)se;
 		kc.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
 		kc.access = ec->hdr.access;
@@ -1105,8 +1105,8 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
 			SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
 			return -EINVAL;
 
-		route.source = elem->source;
-		route.sink = elem->sink;
+		route.source = kstrdup(elem->source, GFP_KERNEL);
+		route.sink = kstrdup(elem->sink, GFP_KERNEL);
 		route.connected = NULL; /* set to NULL atm for tplg users */
 		if (strnlen(elem->control, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) == 0)
 			route.control = NULL;
@@ -1149,7 +1149,7 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_dmixer_create(
 		dev_dbg(tplg->dev, " adding DAPM widget mixer control %s at %d\n",
 			mc->hdr.name, i);
 
-		kc[i].name = mc->hdr.name;
+		kc[i].name = kstrdup(mc->hdr.name, GFP_KERNEL);
 		kc[i].private_value = (long)sm;
 		kc[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
 		kc[i].access = mc->hdr.access;
@@ -1228,7 +1228,7 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_denum_create(
 	dev_dbg(tplg->dev, " adding DAPM widget enum control %s\n",
 		ec->hdr.name);
 
-	kc->name = ec->hdr.name;
+	kc->name = kstrdup(ec->hdr.name, GFP_KERNEL);
 	kc->private_value = (long)se;
 	kc->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
 	kc->access = ec->hdr.access;
@@ -1330,7 +1330,7 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_dbytes_create(
 			"ASoC: adding bytes kcontrol %s with access 0x%x\n",
 			be->hdr.name, be->hdr.access);
 
-		kc[i].name = be->hdr.name;
+		kc[i].name = kstrdup(be->hdr.name, GFP_KERNEL);
 		kc[i].private_value = (long)sbe;
 		kc[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
 		kc[i].access = be->hdr.access;
-- 
1.9.1

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

[Vinod Koul]

On Thu, Nov 26, 2015 at 12:46:24PM +0100, Takashi Iwai wrote:

> > 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.

It is the latter dapm case with added complexity of topology core creating
these kcontrols. I will reproduce this and send the oops tomorrow

-- 
~Vinod

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

[Takashi Iwai]

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:
> 
> > > 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.
> 
> 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);
 	}
 	/* widget w is freed by soc-dapm.c */
 }
@@ -1149,7 +1159,9 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_dmixer_create(
 		dev_dbg(tplg->dev, " adding DAPM widget mixer control %s at %d\n",
 			mc->hdr.name, i);
 
-		kc[i].name = mc->hdr.name;
+		kc[i].name = kstrdup(mc->hdr.name, GFP_KERNEL);
+		if (!kc[i].name)
+			goto err_str;
 		kc[i].private_value = (long)sm;
 		kc[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
 		kc[i].access = mc->hdr.access;
@@ -1196,7 +1208,7 @@ err_str:
 err:
 	for (--i; i >= 0; i--)
 		kfree((void *)kc[i].private_value);
-	kfree(kc);
+	free_kcontrol_news(kc, num_kcontrols);
 	return NULL;
 }
 
@@ -1228,7 +1240,9 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_denum_create(
 	dev_dbg(tplg->dev, " adding DAPM widget enum control %s\n",
 		ec->hdr.name);
 
-	kc->name = ec->hdr.name;
+	kc->name = kstrdup(ec->hdr.name, GFP_KERNEL);
+	if (!kc->name)
+		goto err;
 	kc->private_value = (long)se;
 	kc->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
 	kc->access = ec->hdr.access;
@@ -1294,7 +1308,7 @@ err_se:
 
 	kfree(se);
 err:
-	kfree(kc);
+	free_kcontrol_news(kc, 1);
 
 	return NULL;
 }
@@ -1330,7 +1344,9 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_dbytes_create(
 			"ASoC: adding bytes kcontrol %s with access 0x%x\n",
 			be->hdr.name, be->hdr.access);
 
-		kc[i].name = be->hdr.name;
+		kc[i].name = kstrdup(be->hdr.name, GFP_KERNEL);
+		if (!kc[i].name)
+			goto err;
 		kc[i].private_value = (long)sbe;
 		kc[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
 		kc[i].access = be->hdr.access;
@@ -1363,7 +1379,7 @@ err:
 	for (--i; i >= 0; i--)
 		kfree((void *)kc[i].private_value);
 
-	kfree(kc);
+	free_kcontrol_news(kc, count);
 	return NULL;
 }
 

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

[Takashi Iwai]

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:
> > > 
> > > > > 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.
> > > 
> > > 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.


Takashi

> 
> Regards,
> Subhransu
> >  	}
> >  	/* widget w is freed by soc-dapm.c */
> >  }
> > @@ -1149,7 +1159,9 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_dmixer_create(
> >  		dev_dbg(tplg->dev, " adding DAPM widget mixer control %s at %d\n",
> >  			mc->hdr.name, i);
> >  
> > -		kc[i].name = mc->hdr.name;
> > +		kc[i].name = kstrdup(mc->hdr.name, GFP_KERNEL);
> > +		if (!kc[i].name)
> > +			goto err_str;
> >  		kc[i].private_value = (long)sm;
> >  		kc[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
> >  		kc[i].access = mc->hdr.access;
> > @@ -1196,7 +1208,7 @@ err_str:
> >  err:
> >  	for (--i; i >= 0; i--)
> >  		kfree((void *)kc[i].private_value);
> > -	kfree(kc);
> > +	free_kcontrol_news(kc, num_kcontrols);
> >  	return NULL;
> >  }
> >  
> > @@ -1228,7 +1240,9 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_denum_create(
> >  	dev_dbg(tplg->dev, " adding DAPM widget enum control %s\n",
> >  		ec->hdr.name);
> >  
> > -	kc->name = ec->hdr.name;
> > +	kc->name = kstrdup(ec->hdr.name, GFP_KERNEL);
> > +	if (!kc->name)
> > +		goto err;
> >  	kc->private_value = (long)se;
> >  	kc->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
> >  	kc->access = ec->hdr.access;
> > @@ -1294,7 +1308,7 @@ err_se:
> >  
> >  	kfree(se);
> >  err:
> > -	kfree(kc);
> > +	free_kcontrol_news(kc, 1);
> >  
> >  	return NULL;
> >  }
> > @@ -1330,7 +1344,9 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_dbytes_create(
> >  			"ASoC: adding bytes kcontrol %s with access 0x%x\n",
> >  			be->hdr.name, be->hdr.access);
> >  
> > -		kc[i].name = be->hdr.name;
> > +		kc[i].name = kstrdup(be->hdr.name, GFP_KERNEL);
> > +		if (!kc[i].name)
> > +			goto err;
> >  		kc[i].private_value = (long)sbe;
> >  		kc[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
> >  		kc[i].access = be->hdr.access;
> > @@ -1363,7 +1379,7 @@ err:
> >  	for (--i; i >= 0; i--)
> >  		kfree((void *)kc[i].private_value);
> >  
> > -	kfree(kc);
> > +	free_kcontrol_news(kc, count);
> >  	return NULL;
> >  }
> >  
> 
> -- 
> 

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

[Takashi Iwai]

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

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

[Subhransu S. Prusty]

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:
> > 
> > > > 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.
> > 
> > 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.

Otherwise shouldn't the devm version of kstrdup work good as it just frees
the memory when the device is removed?

Regards,
Subhransu
>  	}
>  	/* widget w is freed by soc-dapm.c */
>  }
> @@ -1149,7 +1159,9 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_dmixer_create(
>  		dev_dbg(tplg->dev, " adding DAPM widget mixer control %s at %d\n",
>  			mc->hdr.name, i);
>  
> -		kc[i].name = mc->hdr.name;
> +		kc[i].name = kstrdup(mc->hdr.name, GFP_KERNEL);
> +		if (!kc[i].name)
> +			goto err_str;
>  		kc[i].private_value = (long)sm;
>  		kc[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
>  		kc[i].access = mc->hdr.access;
> @@ -1196,7 +1208,7 @@ err_str:
>  err:
>  	for (--i; i >= 0; i--)
>  		kfree((void *)kc[i].private_value);
> -	kfree(kc);
> +	free_kcontrol_news(kc, num_kcontrols);
>  	return NULL;
>  }
>  
> @@ -1228,7 +1240,9 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_denum_create(
>  	dev_dbg(tplg->dev, " adding DAPM widget enum control %s\n",
>  		ec->hdr.name);
>  
> -	kc->name = ec->hdr.name;
> +	kc->name = kstrdup(ec->hdr.name, GFP_KERNEL);
> +	if (!kc->name)
> +		goto err;
>  	kc->private_value = (long)se;
>  	kc->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
>  	kc->access = ec->hdr.access;
> @@ -1294,7 +1308,7 @@ err_se:
>  
>  	kfree(se);
>  err:
> -	kfree(kc);
> +	free_kcontrol_news(kc, 1);
>  
>  	return NULL;
>  }
> @@ -1330,7 +1344,9 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_dbytes_create(
>  			"ASoC: adding bytes kcontrol %s with access 0x%x\n",
>  			be->hdr.name, be->hdr.access);
>  
> -		kc[i].name = be->hdr.name;
> +		kc[i].name = kstrdup(be->hdr.name, GFP_KERNEL);
> +		if (!kc[i].name)
> +			goto err;
>  		kc[i].private_value = (long)sbe;
>  		kc[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
>  		kc[i].access = be->hdr.access;
> @@ -1363,7 +1379,7 @@ err:
>  	for (--i; i >= 0; i--)
>  		kfree((void *)kc[i].private_value);
>  
> -	kfree(kc);
> +	free_kcontrol_news(kc, count);
>  	return NULL;
>  }
>  

-- 

Re: [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
> 
> > 
> > -- 
> > 

--