| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 15 Jul 2010 15:42:07 +0800
From: Axel Lin <axel.lin@...il.com>
To: Guennadi Liakhovetski <g.liakhovetski@....de>
Cc: linux-kernel <linux-kernel@...r.kernel.org>,
Liam Girdwood <lrg@...mlogic.co.uk>,
Mark Brown <broonie@...nsource.wolfsonmicro.com>,
Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.de>,
Tejun Heo <tj@...nel.org>, alsa-devel@...a-project.org
Subject: Re: [PATCH 7/12] wm8940: fix resource reclaim in wm8940_register
error path
Hi Guennadi,
2010/7/15 Guennadi Liakhovetski <g.liakhovetski@....de>:
> Hi Axel
>
> On Thu, 15 Jul 2010, Axel Lin wrote:
>
>> >>From 44c93254e5158e3086c4707148d24746067f6b14 Mon Sep 17 00:00:00 2001
>> From: Axel Lin <axel.lin@...il.com>
>> Date: Thu, 15 Jul 2010 10:13:34 +0800
>> Subject: [PATCH] wm8940: fix resource reclaim in wm8940_register error path
>>
>> This patch fixes the error path in wm8940_register to properly free resources.
>>
>> Signed-off-by: Axel Lin <axel.lin@...il.com>
>
> Ok, these bugs are real, but let's fix them properly, i.e., release
> resources at the same level, where they have been acquired:
OK.
I'll send a revised patch now.
I change the patch titile to:
[PATCH v2 7/12] wm8940: fix a memory leak if wm8940_register return error
>
> static int register_codec(struct drv_priv *data)
> {
> int ret = snd_soc_register_codec();
> if (ret < 0)
> return ret;
>
> ret = try();
> if (failed)
> goto err;
>
> ...
>
> err:
> snd_soc_unregister_codec();
> }
>
> static int probe()
> {
> int ret;
> struct drv_priv *data = kzalloc(sizeof(*data), GFP_KERNEL);
> if (!data)
> return -ENOMEM;
>
> ret = register_codec(data);
> if (ret < 0)
> goto err;
>
> ...
>
> err:
> kfree(data);
> return ret;
> }
>
> That is, kfree() belongs to probe() and not to register_codec. This, of
> course, concerns all aour patches, at least those of them, that I have
> seen.
I agree that it is good to release resources at the same level,
where they have been acquired.
But I prefer to follow the maintainer/author's coding style.
For the changes in other drivers, I'd like to see Liam and Mark's comments.
Or if the driver maintainers request it, I can fix it.
Regards,
Axel
>
> Thanks
> Guennadi
>
>> ---
>> sound/soc/codecs/wm8940.c | 22 ++++++++++++++--------
>> 1 files changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/sound/soc/codecs/wm8940.c b/sound/soc/codecs/wm8940.c
>> index e3c4bbf..9673e6f 100644
>> --- a/sound/soc/codecs/wm8940.c
>> +++ b/sound/soc/codecs/wm8940.c
>> @@ -766,7 +766,8 @@ static int wm8940_register(struct wm8940_priv *wm8940,
>> u16 reg;
>> if (wm8940_codec) {
>> dev_err(codec->dev, "Another WM8940 is registered\n");
>> - return -EINVAL;
>> + ret = -EINVAL;
>> + goto err;
>> }
>>
>> INIT_LIST_HEAD(&codec->dapm_widgets);
>> @@ -785,7 +786,7 @@ static int wm8940_register(struct wm8940_priv *wm8940,
>> ret = snd_soc_codec_set_cache_io(codec, 8, 16, control);
>> if (ret < 0) {
>> dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
>> - return ret;
>> + goto err;
>> }
>>
>> memcpy(codec->reg_cache, wm8940_reg_defaults,
>> @@ -794,7 +795,7 @@ static int wm8940_register(struct wm8940_priv *wm8940,
>> ret = wm8940_reset(codec);
>> if (ret < 0) {
>> dev_err(codec->dev, "Failed to issue reset\n");
>> - return ret;
>> + goto err;
>> }
>>
>> wm8940_dai.dev = codec->dev;
>> @@ -803,7 +804,7 @@ static int wm8940_register(struct wm8940_priv *wm8940,
>>
>> ret = snd_soc_write(codec, WM8940_POWER1, 0x180);
>> if (ret < 0)
>> - return ret;
>> + goto err;
>>
>> if (!pdata)
>> dev_warn(codec->dev, "No platform data supplied\n");
>> @@ -811,7 +812,7 @@ static int wm8940_register(struct wm8940_priv *wm8940,
>> reg = snd_soc_read(codec, WM8940_OUTPUTCTL);
>> ret = snd_soc_write(codec, WM8940_OUTPUTCTL, reg | pdata->vroi);
>> if (ret < 0)
>> - return ret;
>> + goto err;
>> }
>>
>>
>> @@ -820,17 +821,22 @@ static int wm8940_register(struct wm8940_priv *wm8940,
>> ret = snd_soc_register_codec(codec);
>> if (ret) {
>> dev_err(codec->dev, "Failed to register codec: %d\n", ret);
>> - return ret;
>> + goto err;
>> }
>>
>> ret = snd_soc_register_dai(&wm8940_dai);
>> if (ret) {
>> dev_err(codec->dev, "Failed to register DAI: %d\n", ret);
>> - snd_soc_unregister_codec(codec);
>> - return ret;
>> + goto err_codec;
>> }
>>
>> return 0;
>> +
>> +err_codec:
>> + snd_soc_unregister_codec(codec);
>> +err:
>> + kfree(wm8940);
>> + return ret;
>> }
>>
>> static void wm8940_unregister(struct wm8940_priv *wm8940)
>> --
>> 1.5.4.3
>>
>>
>>
>
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists