From: Lars-Peter Clausen <lars@metafoo.de> To: Olof Johansson <olof@lixom.net> Cc: Dan Carpenter <dan.carpenter@oracle.com>, Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>, Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.de>, "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>, kernel-janitors@vger.kernel.org Subject: Re: [patch] ASoC: dapm: using freed pointer in dapm_kcontrol_add_widget() Date: Wed, 31 Jul 2013 18:44:17 +0000 [thread overview] Message-ID: <51F95B01.7040002@metafoo.de> (raw) In-Reply-To: <51F9585F.8060505@metafoo.de> 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; }
WARNING: multiple messages have this Message-ID (diff)
From: Lars-Peter Clausen <lars@metafoo.de> To: Olof Johansson <olof@lixom.net> Cc: Dan Carpenter <dan.carpenter@oracle.com>, Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>, Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.de>, "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>, kernel-janitors@vger.kernel.org Subject: Re: [patch] ASoC: dapm: using freed pointer in dapm_kcontrol_add_widget() Date: Wed, 31 Jul 2013 20:44:17 +0200 [thread overview] Message-ID: <51F95B01.7040002@metafoo.de> (raw) In-Reply-To: <51F9585F.8060505@metafoo.de> 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; }
next prev parent reply other threads:[~2013-07-31 18:44 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-07-31 8:52 [patch] ASoC: dapm: using freed pointer in dapm_kcontrol_add_widget() Dan Carpenter 2013-07-31 8:52 ` Dan Carpenter 2013-07-31 9:02 ` Lars-Peter Clausen 2013-07-31 9:02 ` Lars-Peter Clausen 2013-07-31 18:17 ` Olof Johansson 2013-07-31 18:17 ` Olof Johansson 2013-07-31 18:33 ` Lars-Peter Clausen 2013-07-31 18:33 ` Lars-Peter Clausen 2013-07-31 18:44 ` Lars-Peter Clausen [this message] 2013-07-31 18:44 ` Lars-Peter Clausen 2013-07-31 20:05 ` Mark Brown 2013-07-31 20:05 ` Mark Brown 2013-08-01 3:49 ` Olof Johansson 2013-08-01 3:49 ` Olof Johansson 2013-07-31 11:19 ` Mark Brown 2013-07-31 11:19 ` Mark Brown
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=51F95B01.7040002@metafoo.de \ --to=lars@metafoo.de \ --cc=alsa-devel@alsa-project.org \ --cc=broonie@kernel.org \ --cc=dan.carpenter@oracle.com \ --cc=kernel-janitors@vger.kernel.org \ --cc=lgirdwood@gmail.com \ --cc=olof@lixom.net \ --cc=perex@perex.cz \ --cc=tiwai@suse.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.