On Tue, Jun 1, 2021 at 10:43 PM Dan Carpenter dan.carpenter@oracle.com 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@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.nameSee 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