[PATCH 1/2] ASoC: topology: Fix some error codes in soc_tplg_dapm_widget_create()

[PATCH 1/2] ASoC: topology: Fix some error codes in soc_tplg_dapm_widget_create()

[Dan Carpenter]

Return -ENOMEM if these allocations fail.  (The current code returns
success).

Fixes: d29d41e28eea ("ASoC: topology: Add support for multiple kcontrol types to a widget")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 sound/soc/soc-topology.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index e71d98d7b116..6b7a813bc264 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -1481,13 +1481,17 @@ static int soc_tplg_dapm_widget_create(struct soc_tplg *tplg,
 
 	template.num_kcontrols = le32_to_cpu(w->num_kcontrols);
 	kc = devm_kcalloc(tplg->dev, le32_to_cpu(w->num_kcontrols), sizeof(*kc), GFP_KERNEL);
-	if (!kc)
+	if (!kc) {
+		ret = -ENOMEM;
 		goto err;
+	}
 
 	kcontrol_type = devm_kcalloc(tplg->dev, le32_to_cpu(w->num_kcontrols), sizeof(unsigned int),
 				     GFP_KERNEL);
-	if (!kcontrol_type)
+	if (!kcontrol_type) {
+		ret = -ENOMEM;
 		goto err;
+	}
 
 	for (i = 0; i < w->num_kcontrols; i++) {
 		control_hdr = (struct snd_soc_tplg_ctl_hdr *)tplg->pos;
-- 
2.30.2

[PATCH 1/2] ASoC: topology: Fix some error codes in soc_tplg_dapm_widget_create()

[Dan Carpenter]

Return -ENOMEM if these allocations fail.  (The current code returns
success).

Fixes: d29d41e28eea ("ASoC: topology: Add support for multiple kcontrol types to a widget")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 sound/soc/soc-topology.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index e71d98d7b116..6b7a813bc264 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -1481,13 +1481,17 @@ static int soc_tplg_dapm_widget_create(struct soc_tplg *tplg,
 
 	template.num_kcontrols = le32_to_cpu(w->num_kcontrols);
 	kc = devm_kcalloc(tplg->dev, le32_to_cpu(w->num_kcontrols), sizeof(*kc), GFP_KERNEL);
-	if (!kc)
+	if (!kc) {
+		ret = -ENOMEM;
 		goto err;
+	}
 
 	kcontrol_type = devm_kcalloc(tplg->dev, le32_to_cpu(w->num_kcontrols), sizeof(unsigned int),
 				     GFP_KERNEL);
-	if (!kcontrol_type)
+	if (!kcontrol_type) {
+		ret = -ENOMEM;
 		goto err;
+	}
 
 	for (i = 0; i < w->num_kcontrols; i++) {
 		control_hdr = (struct snd_soc_tplg_ctl_hdr *)tplg->pos;
-- 
2.30.2

[PATCH 2/2] ASoC: topology: Fix a small leak in soc_tplg_dapm_graph_elems_load()

[Dan Carpenter]

We have to kfree(routes) on this error path.

Fixes: ff9226224437 ("ASoC: topology: Change allocations to resource managed")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 sound/soc/soc-topology.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 6b7a813bc264..5730fcaa7bc6 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -1135,8 +1135,10 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
 	 */
 	for (i = 0; i < count; i++) {
 		routes[i] = devm_kzalloc(tplg->dev, sizeof(*routes[i]), GFP_KERNEL);
-		if (!routes[i])
-			return -ENOMEM;
+		if (!routes[i]) {
+			ret = -ENOMEM;
+			goto free_routes;
+		}
 	}
 
 	for (i = 0; i < count; i++) {
@@ -1198,6 +1200,7 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
 	 * The memory allocated for each dapm route will be freed
 	 * when it is removed in remove_route().
 	 */
+free_routes:
 	kfree(routes);
 
 	return ret;
-- 
2.30.2

[PATCH 2/2] ASoC: topology: Fix a small leak in soc_tplg_dapm_graph_elems_load()

[Dan Carpenter]

We have to kfree(routes) on this error path.

Fixes: ff9226224437 ("ASoC: topology: Change allocations to resource managed")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 sound/soc/soc-topology.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 6b7a813bc264..5730fcaa7bc6 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -1135,8 +1135,10 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
 	 */
 	for (i = 0; i < count; i++) {
 		routes[i] = devm_kzalloc(tplg->dev, sizeof(*routes[i]), GFP_KERNEL);
-		if (!routes[i])
-			return -ENOMEM;
+		if (!routes[i]) {
+			ret = -ENOMEM;
+			goto free_routes;
+		}
 	}
 
 	for (i = 0; i < count; i++) {
@@ -1198,6 +1200,7 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
 	 * The memory allocated for each dapm route will be freed
 	 * when it is removed in remove_route().
 	 */
+free_routes:
 	kfree(routes);
 
 	return ret;
-- 
2.30.2

Re: [PATCH 2/2] ASoC: topology: Fix a small leak in soc_tplg_dapm_graph_elems_load()

[Amadeusz Sławiński]

On 5/20/2021 7:07 AM, Dan Carpenter wrote:
> We have to kfree(routes) on this error path.
> 
> Fixes: ff9226224437 ("ASoC: topology: Change allocations to resource managed")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>   sound/soc/soc-topology.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> index 6b7a813bc264..5730fcaa7bc6 100644
> --- a/sound/soc/soc-topology.c
> +++ b/sound/soc/soc-topology.c
> @@ -1135,8 +1135,10 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
>   	 */
>   	for (i = 0; i < count; i++) {
>   		routes[i] = devm_kzalloc(tplg->dev, sizeof(*routes[i]), GFP_KERNEL);
> -		if (!routes[i])
> -			return -ENOMEM;
> +		if (!routes[i]) {
> +			ret = -ENOMEM;
> +			goto free_routes;
> +		}
>   	}
>   
>   	for (i = 0; i < count; i++) {
> @@ -1198,6 +1200,7 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
>   	 * The memory allocated for each dapm route will be freed
>   	 * when it is removed in remove_route().
>   	 */
> +free_routes:
>   	kfree(routes);
>   
>   	return ret;
> 

Yes, that's right, however looking at this function again, I wonder if 
instead we can just get rid of the routes array and kcalloc call?

Something along those lines (hope that copy paste won't mess it up):



diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 73076d425efb..13db9cfe223f 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -1104,7 +1104,7 @@ static int soc_tplg_dapm_graph_elems_load(struct 
soc_tplg *tplg,
  {
         struct snd_soc_dapm_context *dapm = &tplg->comp->dapm;
         struct snd_soc_tplg_dapm_graph_elem *elem;
-       struct snd_soc_dapm_route **routes;
+       struct snd_soc_dapm_route *route;
         int count, i;
         int ret = 0;

@@ -1122,28 +1122,15 @@ static int soc_tplg_dapm_graph_elems_load(struct 
soc_tplg *tplg,
         dev_dbg(tplg->dev, "ASoC: adding %d DAPM routes for index 
%d\n", count,
                 hdr->index);

-       /* allocate memory for pointer to array of dapm routes */
-       routes = kcalloc(count, sizeof(struct snd_soc_dapm_route *),
-                        GFP_KERNEL);
-       if (!routes)
-               return -ENOMEM;
-
-       /*
-        * allocate memory for each dapm route in the array.
-        * This needs to be done individually so that
-        * each route can be freed when it is removed in remove_route().
-        */
         for (i = 0; i < count; i++) {
-               routes[i] = devm_kzalloc(tplg->dev, sizeof(*routes[i]), 
GFP_KERNEL);
-               if (!routes[i])
+               route = devm_kzalloc(tplg->dev, sizeof(*route), GFP_KERNEL);
+               if (!route)
                         return -ENOMEM;
-       }

-       for (i = 0; i < count; i++) {
                 elem = (struct snd_soc_tplg_dapm_graph_elem *)tplg->pos;
                 tplg->pos += sizeof(struct snd_soc_tplg_dapm_graph_elem);

-               /* validate routes */
+               /* validate route */
                 if (strnlen(elem->source, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
                             SNDRV_CTL_ELEM_ID_NAME_MAXLEN) {
                         ret = -EINVAL;
@@ -1160,46 +1147,32 @@ static int soc_tplg_dapm_graph_elems_load(struct 
soc_tplg *tplg,
                         break;
                 }

-               routes[i]->source = elem->source;
-               routes[i]->sink = elem->sink;
+               route->source = elem->source;
+               route->sink = elem->sink;

                 /* set to NULL atm for tplg users */
-               routes[i]->connected = NULL;
+               route->connected = NULL;
                 if (strnlen(elem->control, 
SNDRV_CTL_ELEM_ID_NAME_MAXLEN) == 0)
-                       routes[i]->control = NULL;
+                       route->control = NULL;
                 else
-                       routes[i]->control = elem->control;
+                       route->control = elem->control;

                 /* add route dobj to dobj_list */
-               routes[i]->dobj.type = SND_SOC_DOBJ_GRAPH;
-               routes[i]->dobj.ops = tplg->ops;
-               routes[i]->dobj.index = tplg->index;
-               list_add(&routes[i]->dobj.list, &tplg->comp->dobj_list);
+               route->dobj.type = SND_SOC_DOBJ_GRAPH;
+               route->dobj.ops = tplg->ops;
+               route->dobj.index = tplg->index;
+               list_add(&route->dobj.list, &tplg->comp->dobj_list);

-               ret = soc_tplg_add_route(tplg, routes[i]);
+               ret = soc_tplg_add_route(tplg, route);
                 if (ret < 0) {
                         dev_err(tplg->dev, "ASoC: topology: add_route 
failed: %d\n", ret);
-                       /*
-                        * this route was added to the list, it will
-                        * be freed in remove_route() so increment the
-                        * counter to skip it in the error handling
-                        * below.
-                        */
-                       i++;
                         break;
                 }

                 /* add route, but keep going if some fail */
-               snd_soc_dapm_add_routes(dapm, routes[i], 1);
+               snd_soc_dapm_add_routes(dapm, route, 1);
         }

-       /*
-        * free pointer to array of dapm routes as this is no longer needed.
-        * The memory allocated for each dapm route will be freed
-        * when it is removed in remove_route().
-        */
-       kfree(routes);
-
         return ret;
  }

Re: [PATCH 2/2] ASoC: topology: Fix a small leak in soc_tplg_dapm_graph_elems_load()

[Amadeusz Sławiński]

On 5/20/2021 7:07 AM, Dan Carpenter wrote:
> We have to kfree(routes) on this error path.
> 
> Fixes: ff9226224437 ("ASoC: topology: Change allocations to resource managed")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>   sound/soc/soc-topology.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> index 6b7a813bc264..5730fcaa7bc6 100644
> --- a/sound/soc/soc-topology.c
> +++ b/sound/soc/soc-topology.c
> @@ -1135,8 +1135,10 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
>   	 */
>   	for (i = 0; i < count; i++) {
>   		routes[i] = devm_kzalloc(tplg->dev, sizeof(*routes[i]), GFP_KERNEL);
> -		if (!routes[i])
> -			return -ENOMEM;
> +		if (!routes[i]) {
> +			ret = -ENOMEM;
> +			goto free_routes;
> +		}
>   	}
>   
>   	for (i = 0; i < count; i++) {
> @@ -1198,6 +1200,7 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
>   	 * The memory allocated for each dapm route will be freed
>   	 * when it is removed in remove_route().
>   	 */
> +free_routes:
>   	kfree(routes);
>   
>   	return ret;
> 

Yes, that's right, however looking at this function again, I wonder if 
instead we can just get rid of the routes array and kcalloc call?

Something along those lines (hope that copy paste won't mess it up):



diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 73076d425efb..13db9cfe223f 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -1104,7 +1104,7 @@ static int soc_tplg_dapm_graph_elems_load(struct 
soc_tplg *tplg,
  {
         struct snd_soc_dapm_context *dapm = &tplg->comp->dapm;
         struct snd_soc_tplg_dapm_graph_elem *elem;
-       struct snd_soc_dapm_route **routes;
+       struct snd_soc_dapm_route *route;
         int count, i;
         int ret = 0;

@@ -1122,28 +1122,15 @@ static int soc_tplg_dapm_graph_elems_load(struct 
soc_tplg *tplg,
         dev_dbg(tplg->dev, "ASoC: adding %d DAPM routes for index 
%d\n", count,
                 hdr->index);

-       /* allocate memory for pointer to array of dapm routes */
-       routes = kcalloc(count, sizeof(struct snd_soc_dapm_route *),
-                        GFP_KERNEL);
-       if (!routes)
-               return -ENOMEM;
-
-       /*
-        * allocate memory for each dapm route in the array.
-        * This needs to be done individually so that
-        * each route can be freed when it is removed in remove_route().
-        */
         for (i = 0; i < count; i++) {
-               routes[i] = devm_kzalloc(tplg->dev, sizeof(*routes[i]), 
GFP_KERNEL);
-               if (!routes[i])
+               route = devm_kzalloc(tplg->dev, sizeof(*route), GFP_KERNEL);
+               if (!route)
                         return -ENOMEM;
-       }

-       for (i = 0; i < count; i++) {
                 elem = (struct snd_soc_tplg_dapm_graph_elem *)tplg->pos;
                 tplg->pos += sizeof(struct snd_soc_tplg_dapm_graph_elem);

-               /* validate routes */
+               /* validate route */
                 if (strnlen(elem->source, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
                             SNDRV_CTL_ELEM_ID_NAME_MAXLEN) {
                         ret = -EINVAL;
@@ -1160,46 +1147,32 @@ static int soc_tplg_dapm_graph_elems_load(struct 
soc_tplg *tplg,
                         break;
                 }

-               routes[i]->source = elem->source;
-               routes[i]->sink = elem->sink;
+               route->source = elem->source;
+               route->sink = elem->sink;

                 /* set to NULL atm for tplg users */
-               routes[i]->connected = NULL;
+               route->connected = NULL;
                 if (strnlen(elem->control, 
SNDRV_CTL_ELEM_ID_NAME_MAXLEN) == 0)
-                       routes[i]->control = NULL;
+                       route->control = NULL;
                 else
-                       routes[i]->control = elem->control;
+                       route->control = elem->control;

                 /* add route dobj to dobj_list */
-               routes[i]->dobj.type = SND_SOC_DOBJ_GRAPH;
-               routes[i]->dobj.ops = tplg->ops;
-               routes[i]->dobj.index = tplg->index;
-               list_add(&routes[i]->dobj.list, &tplg->comp->dobj_list);
+               route->dobj.type = SND_SOC_DOBJ_GRAPH;
+               route->dobj.ops = tplg->ops;
+               route->dobj.index = tplg->index;
+               list_add(&route->dobj.list, &tplg->comp->dobj_list);

-               ret = soc_tplg_add_route(tplg, routes[i]);
+               ret = soc_tplg_add_route(tplg, route);
                 if (ret < 0) {
                         dev_err(tplg->dev, "ASoC: topology: add_route 
failed: %d\n", ret);
-                       /*
-                        * this route was added to the list, it will
-                        * be freed in remove_route() so increment the
-                        * counter to skip it in the error handling
-                        * below.
-                        */
-                       i++;
                         break;
                 }

                 /* add route, but keep going if some fail */
-               snd_soc_dapm_add_routes(dapm, routes[i], 1);
+               snd_soc_dapm_add_routes(dapm, route, 1);
         }

-       /*
-        * free pointer to array of dapm routes as this is no longer needed.
-        * The memory allocated for each dapm route will be freed
-        * when it is removed in remove_route().
-        */
-       kfree(routes);
-
         return ret;
  }

Re: [PATCH 2/2] ASoC: topology: Fix a small leak in soc_tplg_dapm_graph_elems_load()

[Dan Carpenter]

On Thu, May 20, 2021 at 09:54:42AM +0200, Amadeusz Sławiński wrote:
> On 5/20/2021 7:07 AM, Dan Carpenter wrote:
> > We have to kfree(routes) on this error path.
> > 
> > Fixes: ff9226224437 ("ASoC: topology: Change allocations to resource managed")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >   sound/soc/soc-topology.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> > index 6b7a813bc264..5730fcaa7bc6 100644
> > --- a/sound/soc/soc-topology.c
> > +++ b/sound/soc/soc-topology.c
> > @@ -1135,8 +1135,10 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
> >   	 */
> >   	for (i = 0; i < count; i++) {
> >   		routes[i] = devm_kzalloc(tplg->dev, sizeof(*routes[i]), GFP_KERNEL);
> > -		if (!routes[i])
> > -			return -ENOMEM;
> > +		if (!routes[i]) {
> > +			ret = -ENOMEM;
> > +			goto free_routes;
> > +		}
> >   	}
> >   	for (i = 0; i < count; i++) {
> > @@ -1198,6 +1200,7 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
> >   	 * The memory allocated for each dapm route will be freed
> >   	 * when it is removed in remove_route().
> >   	 */
> > +free_routes:
> >   	kfree(routes);
> >   	return ret;
> > 
> 
> Yes, that's right, however looking at this function again, I wonder if
> instead we can just get rid of the routes array and kcalloc call?
> 
> Something along those lines (hope that copy paste won't mess it up):
> 

It did mess it up but I got the idea.  Yeah, that's way better.

regards,
dan carpenter

Re: [PATCH 2/2] ASoC: topology: Fix a small leak in soc_tplg_dapm_graph_elems_load()

[Dan Carpenter]

On Thu, May 20, 2021 at 09:54:42AM +0200, Amadeusz Sławiński wrote:
> On 5/20/2021 7:07 AM, Dan Carpenter wrote:
> > We have to kfree(routes) on this error path.
> > 
> > Fixes: ff9226224437 ("ASoC: topology: Change allocations to resource managed")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >   sound/soc/soc-topology.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> > index 6b7a813bc264..5730fcaa7bc6 100644
> > --- a/sound/soc/soc-topology.c
> > +++ b/sound/soc/soc-topology.c
> > @@ -1135,8 +1135,10 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
> >   	 */
> >   	for (i = 0; i < count; i++) {
> >   		routes[i] = devm_kzalloc(tplg->dev, sizeof(*routes[i]), GFP_KERNEL);
> > -		if (!routes[i])
> > -			return -ENOMEM;
> > +		if (!routes[i]) {
> > +			ret = -ENOMEM;
> > +			goto free_routes;
> > +		}
> >   	}
> >   	for (i = 0; i < count; i++) {
> > @@ -1198,6 +1200,7 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
> >   	 * The memory allocated for each dapm route will be freed
> >   	 * when it is removed in remove_route().
> >   	 */
> > +free_routes:
> >   	kfree(routes);
> >   	return ret;
> > 
> 
> Yes, that's right, however looking at this function again, I wonder if
> instead we can just get rid of the routes array and kcalloc call?
> 
> Something along those lines (hope that copy paste won't mess it up):
> 

It did mess it up but I got the idea.  Yeah, that's way better.

regards,
dan carpenter