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

[Dongliang Mu]

On Tue, Jun 1, 2021 at 10:43 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> On Tue, Jun 01, 2021 at 10:19:22PM +0800, Dongliang Mu wrote:
> > On Tue, Jun 1, 2021 at 9:46 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> > >
> > > On Tue, Jun 01, 2021 at 09:17:04PM +0800, Dongliang Mu wrote:
> > > > On Mon, May 31, 2021 at 7:02 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> > > > > > @@ -701,6 +706,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > > > > >               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;
> > > > > >       }
> > > > >
> > > > > Btw, I have created a Smatch warning for this type of code where we
> > > > > have:
> > > > >
> > > > >         put_device(&foo->dev);
> > > > >         kfree(foo);
> > > >
> > > > I don't think this should be a bug pattern. put_device will drop the
> > > > final reference of one object with struct device and invoke
> > > > device_release to release some resources.
> > > >
> > > > The release function should only clean up the internal resources in
> > > > the device object. It should not touch the led_card which contains the
> > > > device object.
> > > >
> > >
> > > It's only a use after free if you turn CONFIG_DEBUG_KOBJECT_RELEASE
> > > debugging on, which you would never do in a production environment.  The
> > > put_device() function calls kobject_release():
> >
> > This is interesting. Let's dig a little deeper.
> >
> > >
> > > lib/kobject.c
> > >    725  static void kobject_release(struct kref *kref)
> > >    726  {
> > >    727          struct kobject *kobj = container_of(kref, struct kobject, kref);
> > >    728  #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
> > >    729          unsigned long delay = HZ + HZ * (get_random_int() & 0x3);
> > >    730          pr_info("kobject: '%s' (%p): %s, parent %p (delayed %ld)\n",
> > >    731                   kobject_name(kobj), kobj, __func__, kobj->parent, delay);
> > >    732          INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
> > >                                                   ^^^^^^^^^^^^^^^^^^^^^^^
> > >
> > >    733
> > >    734          schedule_delayed_work(&kobj->release, delay);
> > >    735  #else
> > >    736          kobject_cleanup(kobj);
> > >    737  #endif
> > >    738  }
> > >
> > > This release will be done later and it references led_card->dev which is
> > > now freed.
> >
> > The call chain of kobject_delayed_cleanup is kobject_delayed_cleanup
> > -> kobject_cleanup. From the comment, kobject_cleanup should only
> > clean the resources in the kobject, without touching the dev object.
> > To further confirm, I checked the implementation and found out there
> > seem no references to the dev object. Would you mind pointing out the
> > reference to dev object?
>
> The kobj struct is included in the dev struct, it's not a pointer.
>
>         led_card->dev.kobj.name
>
> See all the '.' characters and only one "->"?  If you kfree(led_card)
> then you can't use led_card->dev.kobj any more.

Yeah, you're right. I originally thought the field kobj is a pointer
and there should no problem. Please leave alone the question below. I
thought up this question based on the assumption before.

>
> > Moreover, if kobject_cleanup touches the
> > resources out of kobject, shall we directly change this function other
> > than its callees?
> >
>
> I don't understand your question here.  The rest of the email looks like
> some copy and pasted code but I don't know what I'm supposed to be
> looking for.
>
> I really feel like I have explained things very as well as I can and I'm
> not sure what more I can do to help... :/

You already helped too much, and I learned a lot from the discussion
with you. Don't be bothered by my stupid questions. :)

>
> regards,
> dan carpenter
>