ASoC:topology:fix the oops bug during topology free.

[ghost]

the topology free will cause oops:

1. becasue the snd_soc_dapm_route{} is allocated in stack, which cause
   oops duirng topology free stage. allocate it on the heap and add
   function snd_soc_tplg_route_remove() free function.
2. the string name is allocated in topology's short life buffer,
   which cause oops during topology free stage. re-allocate it on the
   heap can avoid the issue.

Signed-off-by: Wu Zhigang [email protected]

[ghost]

the snd_soc_dapm_route{} instance is allocated in stack in soc_tplg_dapm_graph_elems_load() function. it will not be existed when jump out of this function. but it is linked into the list.
when the access this instance in the free stage. it would hit panic.

[plbossart]

@ranj063 can you please comment on this.

I wonder if this is the right fix. Definitively the practice of using a stack-allocated structure in a linked list is bad, but I don't like the fact that all users of snd_soc_dapm_add_routes() now need to add a free.
It might be a better idea to copy the route structure into a heap-allocated structure in sof_route_load, and free it in sof_route_unload. In other words make the changes local to SOF and not to sound/soc/soc-topology.c

[ghost]

@plbossart
I know your concern.

can we put the whole part below in the sof_route_load() function?
the snd_soc_dapm_route{} instance needs the parameters parsed from tplg.
sof_route_load() needs these parameters to do more searching and configuration.
if yes, we can put the free in the sof_route_unload(), including the snd_sof_route{} and sof_ipc_pipe_comp_connect{} instance. they are allocated in the sof_route_load().

	elem = (struct snd_soc_tplg_dapm_graph_elem *)tplg->pos;
	tplg->pos += sizeof(struct snd_soc_tplg_dapm_graph_elem);

	/* validate routes */
	if (strnlen(elem->source, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
		SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
		return -EINVAL;
	if (strnlen(elem->sink, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
		SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
		return -EINVAL;
	if (strnlen(elem->control, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
		SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
		return -EINVAL;

	route->source = elem->source;
	route->sink = elem->sink;
	route->connected = NULL; /* set to NULL atm for tplg users */
	if (strnlen(elem->control, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) == 0)
		route->control = NULL;
	else
		route->control = elem->control;

[ghost]

@plbossart @lgirdwood @ranj063
there are two panic issues in these code:

1. the variables allocated in the stack, the topology free process will hit the panic for the variables is not flushed after the stack change.
2. the string name should be allocated again. because the memory of the name string will be freed after the topology parsing finished.

I will think about Pierre's concern. Do you have any other suggestion?

[lgirdwood]

no need to check if (ptr) here as kfree() does this already.

[ghost]

@lgirdwood
ok, I will update it after back to office.
thanks

[lgirdwood]

@zhigang-wu this looks fine to me and fix is at correct level. Please fix above comment.

[plbossart]

@lgirdwood I am not ok with the fix, it requires the addition of a free outside of soc-topology.c, so it'd mean taking a risk of breaking other implementations.

[ghost]

@plbossart
Sorry, I have not got your point.
Do you means I should move the kfree() operation in the sof_route_unload() function?

[ghost]

copy the route structure into a heap-allocated structure in sof_route_load

I think it a good way. but if we do this, we have to send "tplg->pos" into the sof_route_load() function.
because in sof_route_load() function, we have to use the route->source and route->sink to search the widget list. then this function's API has to be changed.
is it ok?
thanks

[lgirdwood]

@plbossart ah yes, I misread sof/topology.c for soc/topology.c. The free needs to be in soc/topology.c

[lgirdwood]

For the record this needs to be freed in soc/topology.c NOT sof/topology.c

[lgirdwood]

This should be done is soc/topology.c

[ghost]

@lgirdwood
I am sorry, you mean the "soc/soc-topology.c" ? I did not find the "soc/topology.c" file.
and adding the soc_tplg_remove_route() in the "soc/soc-topology.c" file?
do the kfree() in the soc_tplg_remove_route().
is it right?

[lgirdwood]

@yES, add snd_soc_tplg_remove_route() to soc-topology.c

[ghost]

@lgirdwood
Sorry for the late response for other stuff thing.
I will update it later after finish the test.
Thanks!

[ghost]

@plbossart @lgirdwood
I update the PR. but I do not have the confidence with it. could you give me more suggestions?

1. I add one function snd_soc_tplg_route_remove() in soc-topology.c to free dynamic allocated route.
2. I am not sure this function's operation is suitable, maybe i have to utilize the soc_tplg{}->ops->dapm_route_unload() call-back function? but the input parameters is not suitable for this target.

[lgirdwood]

The commit subject should include the word "fix" to indicate this fixes an existing bug.

The commit message can be change to

Topology free will cause an oops because xxx is on the stack. Fix this by allocating xx on the heap and add a proper xx remove function.

SoB.

[lgirdwood]

Do we need to return int here, use void instead.

[ghost]

@lgirdwood
OK, I will update it.

[ghost]

@lgirdwood
updated the PR's title, updated the commit subject and message already.
thanks

[plbossart]

Your patches are done backwards and need to be repartitioned. You first need to have a patch that only touches sound/soc/soc-topology.c, and then another one that will make use of the new function in sound/soc/sof/topology.c

I am still not clear if this solution is acceptable btw, if there are other users of the function soc_tplg_dapm_graph_elems_load() they would need to call the soc_tplg_remove_route() which doesn't seem quite right

Also fix typos such as "becasue" or "duirng". if you don't have a spell checker in your editor please configure it.

[ghost]

@plbossart
I will update the PR as your suggestion.
Let me explain it first:

1. the function is called in soc_tplg_dapm_graph_elems_load().
   the call-stack is like snd_sof_load_topology() -> .... -> soc_tplg_dapm_graph_elems_load().
   in snd_sof_load_topology(), it allocates a big buffer to hold the whole topology. then do tplg parsing.
   after parsing, the big buffer will be freed when jump out of the snd_sof_load_topology().
   you can check request_firmware and release_firmware to confirm this.
   the route->sink, route->source, route->control points to the content in that buffer(old code's logical).
   In topology free stage, these info will be used. but actual content is gone with the big buffer free.
   the route->sink points to nothing. try to access it will have problem.
2. the route is allocated in stack (the old code logical). the bad things is we call the soc_tplg_add_route() function to add this stack instance into the list. the worse thing is we will use this element in the list during the topology free stage.

Summary:
So from this description, we can see, if other users call the soc_tplg_dapm_graph_elems_load(),
he can not allocate the route in the stack. he must use the heap to malloc it. then our new function
soc_tplg_remove_route() must be called.

I also attention some code use the static table for the route. but they did not call the soc_tplg_dapm_graph_elems_load. and they did not add this route into the list.

[ghost]

@plbossart
I updated the PR already based on your comments.

[lgirdwood]

Where do we unwind this if it fails. i.e say count is 10 and route 8 fails kzalloc. Where do you kfree routes 0 - 7 ?

[ghost]

I think this is a good question.
each time when the route is allocated successfully, it will be linked into the list.
in the topology free stage, the route elem will be got from the list and do the free.
from your question, I think maybe this PR is not enough.
so from your question, I propose a concern of design:
if one of the routes allocated failure, should we free all of them and block this topology parsing or just skip this one and continue for next?

[plbossart]

we want to stop parsing on any error (whether it comes from the topology file or memory allocation). There's no point in trying to conceal and compensate for errors, it's going to lead to more issues.

[lgirdwood]

you should only free the routes allocated by this invocation of this function. In this case routes 0 - 7 and the strdup names for routes 0 - 7 too.

[ghost]

ok. no problem

[lgirdwood]

Need to check return value here and deal with allocation failures.

[ghost]

ok, I will do this.

[lgirdwood]

Can you make sure this is called by other drivers that use topology. Probably used in the driver remove()

[ghost]

@lgirdwood
I have to do more debug on this to confirm this.
But actually I am not clear about your question. the topology parsing only start from the entry point: snd_soc_register_card() at the machine driver register stage.
is there any other driver will do this? the snd_soc_unregister_card()'s call-stack I will confirm whether it will call
snd_sof_free_topology() function or not.

the topology parsing's call stack is:
snd_soc_register_card()
snd_soc_instantiate_card()
soc_probe_link_components()
......
sof_pcm_probe()
snd_sof_load_topology()

[lgirdwood]

I'm asking you to check if any one else will leak memory as a result of this update and if so, they will have call route_free() when they are removed.

[ghost]

I will check it.

[keyonjie]

Hi Zhigang,
you can simple do change as below, that is enough:

/* ASoC SOF DAPM route */
struct snd_sof_route {
struct snd_sof_dev *sdev;

• struct snd_soc_dapm_route *route;

• struct snd_soc_dapm_route route;
  struct list_head list; /* list in sdev route list */

  void *private;
  };

[keyonjie]

-- struct snd_soc_dapm_route *route;

++ struct snd_soc_dapm_route route;

[ghost]

@keyonjie
That is the cool solution to solve this issue.
I will rewrite this PR!
Thanks, Keyon.

[ghost]

@plbossart @lgirdwood
the code I updated already based on your comments.
I will check the code for Liam's question next step.

[lgirdwood]

devm_ should only be used for allocating resources that will be automatically freed at remove(). This cant be used here as topology can be unloaded and then reloaded before remove(). i.e. topology is a runtime resource that will need to be manually managed.
This code still does not check the result of the allocations.

[ghost]

@lgirdwood
Sorry for missing the check.
If we have to free it manually, it should be in the snd_sof_free_topology().
I think other project will not do it like us.
In some code, the route is defined in static table, they did not need this kind of free,
because they did not do it like us to attach the route in the list.

If we allocate the buffer in sof_route_load() and need to free manually in future.
I do not think we could use soc_tplg{}->ops->dapm_route_unload() to free,
although we allocate this in soc_tplg{}->ops->dapm_route_load() function.
because the input parameter of this function we can not used in free stage.
the soc_tplg{} instance is free already after the topology is finished parsing.

Do you have any idea about this?

[ghost]

This PR miss the part of free operation in the topology free stage.
I am thinking about how to find a better way to realize it.

[ghost]

@lgirdwood
I add static function sof_route_remove() in the topology.c to free the allocated buffer in the route.
now we allocate and free in the same level.
I think maybe this change will not affect other project's driver.
the snd_sof_free_topology() is used by us, even other project use this,
if they did not attach the route in the route_list. it will not have problem.

One suggestion, could I revert back the definition of snd_sof_route{}?
and allocate the snd_soc_dapm_route{} in the sof_route_load() function.
and free the route in sof_route_remove() function.

[lgirdwood]

@zhigang-wu can you send new PR that fixes spcm_bind errors that don't free source or connect in sof_route_load()

[ghost]

@lgirdwood
I will check it and do this. thank you!