| Message ID | 20221027013438.991920-1-yangyingliang@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4a4c8482e370d697738a78dcd7bf2780832cb712 |
| Headers | show |
| Series | ALSA: aoa: i2sbus: fix possible memory leak in i2sbus_add_dev() | expand |
On Thu, 27 Oct 2022 03:34:38 +0200, Yang Yingliang wrote: > > dev_set_name() in soundbus_add_one() allocates memory for name, it need be > freed when of_device_register() fails, call soundbus_dev_put() to give up > the reference that hold in device_initialize(), so that it can be freed in > kobject_cleanup() when the refcount hit to 0. And other resources are also > freed in i2sbus_release_dev(), so it can return 0 directly. > > Fixes: f3d9478b2ce4 ("[ALSA] snd-aoa: add snd-aoa") > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> The check of kobj state is awkward, but it seems to be the simplest way... Applied now. Thanks! Takashi > --- > sound/aoa/soundbus/i2sbus/core.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/sound/aoa/soundbus/i2sbus/core.c b/sound/aoa/soundbus/i2sbus/core.c > index faf6b03131ee..f6841daf9e3b 100644 > --- a/sound/aoa/soundbus/i2sbus/core.c > +++ b/sound/aoa/soundbus/i2sbus/core.c > @@ -302,6 +302,10 @@ static int i2sbus_add_dev(struct macio_dev *macio, > > if (soundbus_add_one(&dev->sound)) { > printk(KERN_DEBUG "i2sbus: device registration error!\n"); > + if (dev->sound.ofdev.dev.kobj.state_initialized) { > + soundbus_dev_put(&dev->sound); > + return 0; > + } > goto err; > } > > -- > 2.25.1 >
On 2022/10/27 14:38, Takashi Iwai wrote: > On Thu, 27 Oct 2022 03:34:38 +0200, > Yang Yingliang wrote: >> dev_set_name() in soundbus_add_one() allocates memory for name, it need be >> freed when of_device_register() fails, call soundbus_dev_put() to give up >> the reference that hold in device_initialize(), so that it can be freed in >> kobject_cleanup() when the refcount hit to 0. And other resources are also >> freed in i2sbus_release_dev(), so it can return 0 directly. >> >> Fixes: f3d9478b2ce4 ("[ALSA] snd-aoa: add snd-aoa") >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > The check of kobj state is awkward, but it seems to be the simplest > way... Applied now. Thanks! Indeed, it's awkward, shall we introduce a helper like this: diff --git a/sound/aoa/soundbus/i2sbus/core.c b/sound/aoa/soundbus/i2sbus/core.c index f6841daf9e3b..950c37e0297e 100644 --- a/sound/aoa/soundbus/i2sbus/core.c +++ b/sound/aoa/soundbus/i2sbus/core.c @@ -302,7 +302,7 @@ static int i2sbus_add_dev(struct macio_dev *macio, if (soundbus_add_one(&dev->sound)) { printk(KERN_DEBUG "i2sbus: device registration error!\n"); - if (dev->sound.ofdev.dev.kobj.state_initialized) { + if (soundbus_dev_initialized(&dev->sound)) { soundbus_dev_put(&dev->sound); return 0; } diff --git a/sound/aoa/soundbus/soundbus.h b/sound/aoa/soundbus/soundbus.h index 3a99c1f1a3ca..2c9c95cf156a 100644 --- a/sound/aoa/soundbus/soundbus.h +++ b/sound/aoa/soundbus/soundbus.h @@ -174,6 +174,10 @@ struct soundbus_dev { extern int soundbus_add_one(struct soundbus_dev *dev); extern void soundbus_remove_one(struct soundbus_dev *dev); +static inline bool soundbus_dev_initialized(struct soundbus_dev *dev) +{ + return dev->ofdev.dev.kobj.state_initialized; +} Thanks, Yang > > > Takashi > > >> --- >> sound/aoa/soundbus/i2sbus/core.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/sound/aoa/soundbus/i2sbus/core.c b/sound/aoa/soundbus/i2sbus/core.c >> index faf6b03131ee..f6841daf9e3b 100644 >> --- a/sound/aoa/soundbus/i2sbus/core.c >> +++ b/sound/aoa/soundbus/i2sbus/core.c >> @@ -302,6 +302,10 @@ static int i2sbus_add_dev(struct macio_dev *macio, >> >> if (soundbus_add_one(&dev->sound)) { >> printk(KERN_DEBUG "i2sbus: device registration error!\n"); >> + if (dev->sound.ofdev.dev.kobj.state_initialized) { >> + soundbus_dev_put(&dev->sound); >> + return 0; >> + } >> goto err; >> } >> >> -- >> 2.25.1 >> > .
On Thu, 27 Oct 2022 09:41:03 +0200, Yang Yingliang wrote: > > > On 2022/10/27 14:38, Takashi Iwai wrote: > > On Thu, 27 Oct 2022 03:34:38 +0200, > > Yang Yingliang wrote: > >> dev_set_name() in soundbus_add_one() allocates memory for name, it need be > >> freed when of_device_register() fails, call soundbus_dev_put() to give up > >> the reference that hold in device_initialize(), so that it can be freed in > >> kobject_cleanup() when the refcount hit to 0. And other resources are also > >> freed in i2sbus_release_dev(), so it can return 0 directly. > >> > >> Fixes: f3d9478b2ce4 ("[ALSA] snd-aoa: add snd-aoa") > >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > > The check of kobj state is awkward, but it seems to be the simplest > > way... Applied now. Thanks! > > Indeed, it's awkward, shall we introduce a helper like this: > > diff --git a/sound/aoa/soundbus/i2sbus/core.c > b/sound/aoa/soundbus/i2sbus/core.c > index f6841daf9e3b..950c37e0297e 100644 > --- a/sound/aoa/soundbus/i2sbus/core.c > +++ b/sound/aoa/soundbus/i2sbus/core.c > @@ -302,7 +302,7 @@ static int i2sbus_add_dev(struct macio_dev *macio, > > if (soundbus_add_one(&dev->sound)) { > printk(KERN_DEBUG "i2sbus: device registration error!\n"); > - if (dev->sound.ofdev.dev.kobj.state_initialized) { > + if (soundbus_dev_initialized(&dev->sound)) { > soundbus_dev_put(&dev->sound); > return 0; > } > diff --git a/sound/aoa/soundbus/soundbus.h b/sound/aoa/soundbus/soundbus.h > index 3a99c1f1a3ca..2c9c95cf156a 100644 > --- a/sound/aoa/soundbus/soundbus.h > +++ b/sound/aoa/soundbus/soundbus.h > @@ -174,6 +174,10 @@ struct soundbus_dev { > > extern int soundbus_add_one(struct soundbus_dev *dev); > extern void soundbus_remove_one(struct soundbus_dev *dev); > +static inline bool soundbus_dev_initialized(struct soundbus_dev *dev) > +{ > + return dev->ofdev.dev.kobj.state_initialized; > +} I think it's not worth much as it's used only at a single place. Takashi
diff --git a/sound/aoa/soundbus/i2sbus/core.c b/sound/aoa/soundbus/i2sbus/core.c index faf6b03131ee..f6841daf9e3b 100644 --- a/sound/aoa/soundbus/i2sbus/core.c +++ b/sound/aoa/soundbus/i2sbus/core.c @@ -302,6 +302,10 @@ static int i2sbus_add_dev(struct macio_dev *macio, if (soundbus_add_one(&dev->sound)) { printk(KERN_DEBUG "i2sbus: device registration error!\n"); + if (dev->sound.ofdev.dev.kobj.state_initialized) { + soundbus_dev_put(&dev->sound); + return 0; + } goto err; }
dev_set_name() in soundbus_add_one() allocates memory for name, it need be freed when of_device_register() fails, call soundbus_dev_put() to give up the reference that hold in device_initialize(), so that it can be freed in kobject_cleanup() when the refcount hit to 0. And other resources are also freed in i2sbus_release_dev(), so it can return 0 directly. Fixes: f3d9478b2ce4 ("[ALSA] snd-aoa: add snd-aoa") Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- sound/aoa/soundbus/i2sbus/core.c | 4 ++++ 1 file changed, 4 insertions(+)