Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

From: Dongliang Mu
Date: Mon May 31 2021 - 05:11:21 EST

On Mon, May 31, 2021 at 4:46 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> On Mon, May 31, 2021 at 04:08:04PM +0800, Dongliang Mu wrote:
> > On Mon, May 31, 2021 at 3:34 PM Dongliang Mu <mudongliangabcd@xxxxxxxxx> wrote:
> > >
> > > On Mon, May 31, 2021 at 3:03 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> > > >
> > > > On Mon, May 31, 2021 at 02:20:37PM +0800, Dongliang Mu wrote:
> > > > > On Mon, May 31, 2021 at 12:40 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> > > > > >
> > > > > > On Mon, May 31, 2021 at 11:03:36AM +0800, Dongliang Mu wrote:
> > > > > > > On Sat, May 29, 2021 at 5:35 AM 慕冬亮 <mudongliangabcd@xxxxxxxxx> wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > > On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> > > > > > > > >
> > > > > > > > > On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
> > > > > > > > >>
> > > > > > > > >> Can you please give some advise on how to fix this WARN issue?
> > > > > > > > >
> > > > > > > > > But it feels like it spoils the fun if I write the commit... Anyway:
> > > > > > > >
> > > > > > > > It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > regards,
> > > > > > > > > dan carpenter
> > > > > > > > >
> > > > > > > > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > > > > > > > index 25f57c14f294..dd357abc1b58 100644
> > > > > > > > > --- a/sound/core/control_led.c
> > > > > > > > > +++ b/sound/core/control_led.c
> > > > > > > > > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> > > > > > > > > for (; group > 0; group--) {
> > > > > > > > > led = &snd_ctl_leds[group - 1];
> > > > > > > > > device_del(&led->dev);
> > > > > > > > > + device_put(&led->dev);
> > > > > > > > > }
> > > > > > > > > device_del(&snd_ctl_led_dev);
> > > > > > > > > return -ENOMEM;
> > > > > > > > > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> > > > > > > > > for (group = 0; group < MAX_LED; group++) {
> > > > > > > > > led = &snd_ctl_leds[group];
> > > > > > > > > device_del(&led->dev);
> > > > > > > > > + device_put(&led->dev);
> > > > > > > > > }
> > > > > > > > > device_del(&snd_ctl_led_dev);
> > > > > > > > > snd_ctl_led_clean(NULL);
> > > > > > >
> > > > > > > Hi Dan,
> > > > > > >
> > > > > > > I tried this patch, and it still triggers the memleak.
> > > > > >
> > > > > > Did your patch fix the leak? Because my patch should have been
> > > > > > equivalent except for it fixes an additional leak in the snd_ctl_led_init()
> > > > > > error path.
> > > > >
> > > > > The syzbot link is [1]. I have tested my patch in the syzbot dashboard
> > > > > and my local workspace.
> > > > >
> > > > > I think the reason why your patch did not work should be
> > > > > led_card(struct snd_ctl_led_card) is already freed before returning in
> > > > > snd_ctl_led_sysfs_remove, rather than led(struct snd_ctl_led). See the
> > > > > implementation of snd_ctl_led_sysfs_remove for some details. Please
> > > > > correct me if I make any mistakes.
> > > > >
> > > > > static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > > > > {
> > > > > unsigned int group;
> > > > > struct snd_ctl_led_card *led_card;
> > > > > struct snd_ctl_led *led;
> > > > > char link_name[32];
> > > > >
> > > > > for (group = 0; group < MAX_LED; group++) {
> > > > > led = &snd_ctl_leds[group];
> > > > > led_card = led->cards[card->number];
> > > > > if (!led_card)
> > > > > continue;
> > > > > snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> > > > > sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > > > > sysfs_remove_link(&led_card->dev.kobj, "card");
> > > > > device_del(&led_card->dev);
> > > > > put_device(&led_card->dev);
> > > > > kfree(led_card);
> > > > > led->cards[card->number] = NULL;
> > > > > }
> > > > > }
> > > >
> > > > This is frustrating to look at because it's not a diff so it doesn't
> > > > show what you changed. I think you are saying that you added the
> > > > put_device(&led_card->dev);. That's true. There are some other leaks
> > > > as well. We should just fix them all. Use device_unregister() because
> > > > it's cleaner.
> > >
> > > Oh, I see your point. Yeah, we should fix these memory leaks all. I
> > > agree with device_unregister.
> > >
> > > >
> > > > If both device_initialize() and device_add() succeed then call
> > > > device_unregister() to unwind.
> > >
> > > BTW, have you tested this new patch on two memory leaks?
> > >
> >
> > Please keep in mind that if we don't have any release method for
> > struct snd_ctl_led_card, it will trigger a WARN[1] in the
> > device_release function. That's why I have to add one dummy release
> > function.
> >
> > if (dev->release)
> > dev->release(dev);
> > else if (dev->type && dev->type->release)
> > dev->type->release(dev);
> > else if (dev->class && dev->class->dev_release)
> > dev->class->dev_release(dev);
> > else
> > WARN(1, KERN_ERR "Device '%s' does not have a release()
> > function, it is broken and must be fixed. See
> > Documentation/core-api/kobject.rst.\n",
> > dev_name(dev));
> >
>
> Oh yeah. You're right. The "kfree(led_card);" needs to be moved to a
> release function or it can lead to a use after free. For the others,
> I think a dummy release function is ok (because it is static data).
>

Hi Dan,

I wonder if we shall split the current patch into two patches, one
patch for each memory leak. It is better to satisfy the rule - "one
patch only fixes one issue".

We should absolutely fix all these memory leaks. But one patch for two
different bugs with different objects and different paths is not very
suitable.

> It feels like there should be a standard way to say that there is no
> need to release any data. That way it could be verified by static
> analysis tools.
>
> regards,
> dan carpenter
>
> > [1] https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L2110
> >
> > > >
> > > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > > index 25f57c14f294..561fe45e4449 100644
> > > > --- a/sound/core/control_led.c
> > > > +++ b/sound/core/control_led.c
> > > > @@ -700,7 +700,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > > > snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> > > > sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > > > sysfs_remove_link(&led_card->dev.kobj, "card");
> > > > - device_del(&led_card->dev);
> > > > + device_unregister(&led_card->dev);
> > > > kfree(led_card);
> > > > led->cards[card->number] = NULL;
> > > > }
> > > > @@ -739,9 +739,9 @@ static int __init snd_ctl_led_init(void)
> > > > put_device(&led->dev);
> > > > for (; group > 0; group--) {
> > > > led = &snd_ctl_leds[group - 1];
> > > > - device_del(&led->dev);
> > > > + device_unregister(&led->dev);
> > > > }
> > > > - device_del(&snd_ctl_led_dev);
> > > > + device_unregister(&snd_ctl_led_dev);
> > > > return -ENOMEM;
> > > > }
> > > > }
> > > > @@ -767,9 +767,9 @@ static void __exit snd_ctl_led_exit(void)
> > > > }
> > > > for (group = 0; group < MAX_LED; group++) {
> > > > led = &snd_ctl_leds[group];
> > > > - device_del(&led->dev);
> > > > + device_unregister(&led->dev);
> > > > }
> > > > - device_del(&snd_ctl_led_dev);
> > > > + device_unregister(&snd_ctl_led_dev);
> > > > snd_ctl_led_clean(NULL);
> > > > }
> > > >