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. > 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