From: Dan Carpenter <dan.carpenter@oracle.com> To: Dongliang Mu <mudongliangabcd@gmail.com> Cc: perex@perex.cz, tiwai@suse.com, alsa-devel@alsa-project.org, linux-kernel <linux-kernel@vger.kernel.org>, syzbot+08a7d8b51ea048a74ffb@syzkaller.appspotmail.com Subject: Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register Date: Tue, 1 Jun 2021 17:43:05 +0300 [thread overview] Message-ID: <20210601143711.GE24442@kadam> (raw) In-Reply-To: <CAD-N9QWspFya5YmFsR=9tskS_JK+8V1suuPiC=h2XpPt3=KymQ@mail.gmail.com> 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@oracle.com> 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@oracle.com> 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. > 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... :/ regards, dan carpenter
WARNING: multiple messages have this Message-ID (diff)
From: Dan Carpenter <dan.carpenter@oracle.com> To: Dongliang Mu <mudongliangabcd@gmail.com> Cc: syzbot+08a7d8b51ea048a74ffb@syzkaller.appspotmail.com, linux-kernel <linux-kernel@vger.kernel.org>, alsa-devel@alsa-project.org, tiwai@suse.com Subject: Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register Date: Tue, 1 Jun 2021 17:43:05 +0300 [thread overview] Message-ID: <20210601143711.GE24442@kadam> (raw) In-Reply-To: <CAD-N9QWspFya5YmFsR=9tskS_JK+8V1suuPiC=h2XpPt3=KymQ@mail.gmail.com> 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@oracle.com> 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@oracle.com> 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. > 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... :/ regards, dan carpenter
next prev parent reply other threads:[~2021-06-01 14:44 UTC|newest] Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-28 13:17 [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register Dongliang Mu 2021-05-28 13:33 ` Dan Carpenter 2021-05-28 13:33 ` Dan Carpenter 2021-05-28 13:50 ` Dongliang Mu 2021-05-28 13:50 ` Dongliang Mu 2021-05-28 14:05 ` Dan Carpenter 2021-05-28 14:05 ` Dan Carpenter 2021-05-28 21:35 ` 慕冬亮 2021-05-28 21:35 ` 慕冬亮 2021-05-31 3:03 ` Dongliang Mu 2021-05-31 3:03 ` Dongliang Mu 2021-05-31 4:40 ` Dan Carpenter 2021-05-31 4:40 ` Dan Carpenter 2021-05-31 6:20 ` Dongliang Mu 2021-05-31 6:20 ` Dongliang Mu 2021-05-31 7:03 ` Dan Carpenter 2021-05-31 7:03 ` Dan Carpenter 2021-05-31 7:34 ` Dongliang Mu 2021-05-31 7:34 ` Dongliang Mu 2021-05-31 8:08 ` Dongliang Mu 2021-05-31 8:08 ` Dongliang Mu 2021-05-31 8:46 ` Dan Carpenter 2021-05-31 8:46 ` Dan Carpenter 2021-05-31 9:10 ` Dongliang Mu 2021-05-31 9:10 ` Dongliang Mu 2021-05-31 9:38 ` Dan Carpenter 2021-05-31 9:38 ` Dan Carpenter 2021-05-31 10:35 ` Dongliang Mu 2021-05-31 10:35 ` Dongliang Mu 2021-05-31 10:43 ` Dan Carpenter 2021-05-31 10:43 ` Dan Carpenter 2021-05-31 10:59 ` Dongliang Mu 2021-05-31 10:59 ` Dongliang Mu 2021-05-31 8:12 ` Dan Carpenter 2021-05-31 8:12 ` Dan Carpenter 2021-05-31 4:36 ` Dan Carpenter 2021-05-31 4:36 ` Dan Carpenter 2021-05-31 11:01 ` Dan Carpenter 2021-05-31 11:01 ` Dan Carpenter 2021-05-31 11:07 ` Dan Carpenter 2021-05-31 11:07 ` Dan Carpenter 2021-05-31 11:11 ` Dongliang Mu 2021-05-31 11:11 ` Dongliang Mu 2021-06-01 13:17 ` Dongliang Mu 2021-06-01 13:17 ` Dongliang Mu 2021-06-01 13:46 ` Dan Carpenter 2021-06-01 13:46 ` Dan Carpenter 2021-06-01 14:19 ` Dongliang Mu 2021-06-01 14:19 ` Dongliang Mu 2021-06-01 14:43 ` Dan Carpenter [this message] 2021-06-01 14:43 ` Dan Carpenter 2021-06-01 15:52 ` Dongliang Mu 2021-06-01 15:52 ` Dongliang Mu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210601143711.GE24442@kadam \ --to=dan.carpenter@oracle.com \ --cc=alsa-devel@alsa-project.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mudongliangabcd@gmail.com \ --cc=perex@perex.cz \ --cc=syzbot+08a7d8b51ea048a74ffb@syzkaller.appspotmail.com \ --cc=tiwai@suse.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.