| Message ID | 20221020105937.1448951-1-yangyingliang@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] ASoC: Intel: Skylake: fix possible memory leak in skl_codec_device_init() | expand |
On 2022-10-20 12:59 PM, Yang Yingliang wrote: > If snd_hdac_device_register() fails, 'codec' and name allocated in > dev_set_name() called in snd_hdac_device_init() are leaked. Fix this > by calling put_device(), so they can be freed in snd_hda_codec_dev_release() > and kobject_cleanup(). > > Fixes: e4746d94d00c ("ASoC: Intel: Skylake: Introduce HDA codec init and exit routines") > Fixes: dfe66a18780d ("ALSA: hdac_ext: add extended HDA bus") I do not believe the second Fixes-tag is required as it's not connected directly with the fix you're providing. Tag: Suggested-by: Cezary Rojewski <cezary.rojewski@intel.com> would be most welcome though. Also, if there would happen to be v3, please bundle Skylake and SOF patches together into a single patchset (still not a single patch!). > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> ... > sound/soc/intel/skylake/skl.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c > index bbba2df33aaf..3312b57e3c0c 100644 > --- a/sound/soc/intel/skylake/skl.c > +++ b/sound/soc/intel/skylake/skl.c > @@ -689,11 +689,6 @@ static void load_codec_module(struct hda_codec *codec) > > #endif /* CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC */ > > -static void skl_codec_device_exit(struct device *dev) > -{ > - snd_hdac_device_exit(dev_to_hdac_dev(dev)); > -} > - > static struct hda_codec *skl_codec_device_init(struct hdac_bus *bus, int addr) > { > struct hda_codec *codec; > @@ -706,12 +701,11 @@ static struct hda_codec *skl_codec_device_init(struct hdac_bus *bus, int addr) > } > > codec->core.type = HDA_DEV_ASOC; > - codec->core.dev.release = skl_codec_device_exit; > > ret = snd_hdac_device_register(&codec->core); > if (ret) { > dev_err(bus->dev, "failed to register hdac device\n"); > - snd_hdac_device_exit(&codec->core); > + put_device(&codec->core.dev); > return ERR_PTR(ret); > } > Shy question: why my suggestion seems reasonable, I did not test it yet, proposed it based on static analysis of the code. Did you test it? Regards, Czarek
On 2022/10/20 20:10, Cezary Rojewski wrote: > On 2022-10-20 12:59 PM, Yang Yingliang wrote: >> If snd_hdac_device_register() fails, 'codec' and name allocated in >> dev_set_name() called in snd_hdac_device_init() are leaked. Fix this >> by calling put_device(), so they can be freed in >> snd_hda_codec_dev_release() >> and kobject_cleanup(). >> >> Fixes: e4746d94d00c ("ASoC: Intel: Skylake: Introduce HDA codec init >> and exit routines") >> Fixes: dfe66a18780d ("ALSA: hdac_ext: add extended HDA bus") > > I do not believe the second Fixes-tag is required as it's not > connected directly with the fix you're providing. When the reference of device is leaked, it leads to two memory leak: 'codec' and 'dev->kobj.name' which is allocated in dev_set_name(). The name leak is introduce by the second fix tag. This patch calling put_device() to free both of them. > > Tag: > Suggested-by: Cezary Rojewski <cezary.rojewski@intel.com> > > would be most welcome though. > > Also, if there would happen to be v3, please bundle Skylake and SOF > patches together into a single patchset (still not a single patch!). OK, It's my pleasure. > >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > > ... > >> sound/soc/intel/skylake/skl.c | 8 +------- >> 1 file changed, 1 insertion(+), 7 deletions(-) >> >> diff --git a/sound/soc/intel/skylake/skl.c >> b/sound/soc/intel/skylake/skl.c >> index bbba2df33aaf..3312b57e3c0c 100644 >> --- a/sound/soc/intel/skylake/skl.c >> +++ b/sound/soc/intel/skylake/skl.c >> @@ -689,11 +689,6 @@ static void load_codec_module(struct hda_codec >> *codec) >> #endif /* CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC */ >> -static void skl_codec_device_exit(struct device *dev) >> -{ >> - snd_hdac_device_exit(dev_to_hdac_dev(dev)); >> -} >> - >> static struct hda_codec *skl_codec_device_init(struct hdac_bus >> *bus, int addr) >> { >> struct hda_codec *codec; >> @@ -706,12 +701,11 @@ static struct hda_codec >> *skl_codec_device_init(struct hdac_bus *bus, int addr) >> } >> codec->core.type = HDA_DEV_ASOC; >> - codec->core.dev.release = skl_codec_device_exit; >> ret = snd_hdac_device_register(&codec->core); >> if (ret) { >> dev_err(bus->dev, "failed to register hdac device\n"); >> - snd_hdac_device_exit(&codec->core); >> + put_device(&codec->core.dev); >> return ERR_PTR(ret); >> } > > Shy question: why my suggestion seems reasonable, I did not test it > yet, proposed it based on static analysis of the code. Did you test it? I did it by static detailed analysis, there is a common driver core mechanism to make sure snd_hda_codec_dev_release() is called in device_release() when refcount hit 0, and it's ok to call snd_hda_codec_dev_release(), because the member of codec that need be freed is still null, it won't cause any problem. Could you test this patch if you wish ? Thanks, Yang > > > Regards, > Czarek > > .
On Thu, 20 Oct 2022 18:59:37 +0800, Yang Yingliang wrote: > If snd_hdac_device_register() fails, 'codec' and name allocated in > dev_set_name() called in snd_hdac_device_init() are leaked. Fix this > by calling put_device(), so they can be freed in snd_hda_codec_dev_release() > and kobject_cleanup(). > > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: Intel: Skylake: fix possible memory leak in skl_codec_device_init() commit: 0e213813df02da048ffd22a2c4fac041768ca327 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index bbba2df33aaf..3312b57e3c0c 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -689,11 +689,6 @@ static void load_codec_module(struct hda_codec *codec) #endif /* CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC */ -static void skl_codec_device_exit(struct device *dev) -{ - snd_hdac_device_exit(dev_to_hdac_dev(dev)); -} - static struct hda_codec *skl_codec_device_init(struct hdac_bus *bus, int addr) { struct hda_codec *codec; @@ -706,12 +701,11 @@ static struct hda_codec *skl_codec_device_init(struct hdac_bus *bus, int addr) } codec->core.type = HDA_DEV_ASOC; - codec->core.dev.release = skl_codec_device_exit; ret = snd_hdac_device_register(&codec->core); if (ret) { dev_err(bus->dev, "failed to register hdac device\n"); - snd_hdac_device_exit(&codec->core); + put_device(&codec->core.dev); return ERR_PTR(ret); }
If snd_hdac_device_register() fails, 'codec' and name allocated in dev_set_name() called in snd_hdac_device_init() are leaked. Fix this by calling put_device(), so they can be freed in snd_hda_codec_dev_release() and kobject_cleanup(). Fixes: e4746d94d00c ("ASoC: Intel: Skylake: Introduce HDA codec init and exit routines") Fixes: dfe66a18780d ("ALSA: hdac_ext: add extended HDA bus") Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- v1 -> v2: remove skl_codec_device_exit() and use snd_hda_codec_dev_release() to fix leak --- sound/soc/intel/skylake/skl.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)