On 07/31/2013 08:33 PM, Lars-Peter Clausen wrote:

On 07/31/2013 08:17 PM, Olof Johansson wrote:

...

Hi,

On Wed, Jul 31, 2013 at 2:02 AM, Lars-Peter Clausen lars@metafoo.de wrote:

...

On 07/31/2013 10:52 AM, Dan Carpenter wrote:

...

There is a typo here so we end up using the old freed pointer instead of the newly allocated one. (If the "n" is zero then the code works, obviously).

Signed-off-by: Dan Carpenter dan.carpenter@oracle.com

Thanks.

Acked-by: Lars-Peter Clausen lars@metafoo.de

Olof, can you check whether this fixes the crash you see?

Nope.

There's also remaining issues with the code, that patch isn't enough. The structure that is krealloced() has a list_head in it, but the list isn't moved from the old head to the new one. There's no safe way to do that using krealloc, since the old list_head is gone by then, so it's probably easest to open-code with kzalloc/memcpy/kfree.

Hm, right I didn't think of that. Maybe it's better to just keep a the widget list in a separate pointer, so none of the other fields of the kcontrol_data struct are affected by the krealloc.

Something along the lines of this:

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index d74c356..ef1db0b 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -177,7 +177,7 @@ static inline struct snd_soc_dapm_widget *dapm_cnew_widget( struct dapm_kcontrol_data { unsigned int value; struct list_head paths; - struct snd_soc_dapm_widget_list wlist; + struct snd_soc_dapm_widget_list *wlist; };

static int dapm_kcontrol_data_alloc(struct snd_soc_dapm_widget *widget, @@ -185,26 +185,36 @@ static int dapm_kcontrol_data_alloc(struct { struct dapm_kcontrol_data *data;

- data = kzalloc(sizeof(*data) + sizeof(widget), GFP_KERNEL); - if (!data) { - dev_err(widget->dapm->dev, - "ASoC: can't allocate kcontrol data for %s\n", - widget->name); - return -ENOMEM; - } + data = kzalloc(sizeof(*data), GFP_KERNEL); + if (!data) + goto err;

- data->wlist.widgets[0] = widget; - data->wlist.num_widgets = 1; + data->wlist = kzalloc(sizeof(*data->wlist) + sizeof(widget), + GFP_KERNEL); + if (!data->wlist) + goto err_free; + + data->wlist->widgets[0] = widget; + data->wlist->num_widgets = 1; INIT_LIST_HEAD(&data->paths);

kcontrol->private_data = data;

return 0; +err_free: + kfree(data); +err: + dev_err(widget->dapm->dev, + "ASoC: can't allocate kcontrol data for %s\n", + widget->name); + + return -ENOMEM; }

static void dapm_kcontrol_free(struct snd_kcontrol *kctl) { struct dapm_kcontrol_data *data = snd_kcontrol_chip(kctl); + kfree(data->wlist); kfree(data); }

@@ -213,25 +223,25 @@ static struct snd_soc_dapm_widget_list { struct dapm_kcontrol_data *data = snd_kcontrol_chip(kcontrol);

- return &data->wlist; + return data->wlist; }

static int dapm_kcontrol_add_widget(struct snd_kcontrol *kcontrol, struct snd_soc_dapm_widget *widget) { struct dapm_kcontrol_data *data = snd_kcontrol_chip(kcontrol); - struct dapm_kcontrol_data *new_data; - unsigned int n = data->wlist.num_widgets + 1; + struct snd_soc_dapm_widget_list *new_wlist; + unsigned int n = data->wlist->num_widgets + 1;

- new_data = krealloc(data, sizeof(*data) + sizeof(widget) * n, + new_wlist = krealloc(data, sizeof(*new_wlist) + sizeof(widget) * n, GFP_KERNEL); - if (!new_data) + if (!new_wlist) return -ENOMEM;

- new_data->wlist.widgets[n - 1] = widget; - new_data->wlist.num_widgets = n; + new_wlist->widgets[n - 1] = widget; + new_wlist->num_widgets = n;

- kcontrol->private_data = new_data; + data->wlist = new_wlist;

return 0; }