[PATCH] kobject: Ensure child's resources get released before parent's resources

[PATCH] kobject: Ensure child's resources get released before parent's resources

[Rajat Jain]

If the only remaining reference to a parent, is the one taken by
the child (in kobject_add_internal()), then when the last
reference to the child goes away, both child and its parents
shall be released. However, currently the resources of parent
get released first, followed by the child's resources:

kobject_cleanup(child)
    ....
    kobject_del(child)
        ....
        kobject_put(child->parent) -> results in parent's release()
        ...
    child->kobj_type->release() -> Child's release()

This is a problem because the child's release() method may still
need to use parent resources or memory for its own cleanup. E.g.
child may need parent pointer for dma_free_coherent() etc.

Signed-off-by: Rajat Jain <rajatja@google.com>
Signed-off-by: Rajat Jain <rajatxjain@gmail.com>
---
resending after an email address that bounced.

 lib/kobject.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/kobject.c b/lib/kobject.c
index 7cbccd2..8f325ac 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -616,6 +616,7 @@ static void kobject_cleanup(struct kobject *kobj)
 {
 	struct kobj_type *t = get_ktype(kobj);
 	const char *name = kobj->name;
+	struct kobject *parent = kobj->parent;
 
 	pr_debug("kobject: '%s' (%p): %s, parent %p\n",
 		 kobject_name(kobj), kobj, __func__, kobj->parent);
@@ -632,6 +633,8 @@ static void kobject_cleanup(struct kobject *kobj)
 		kobject_uevent(kobj, KOBJ_REMOVE);
 	}
 
+	kobject_get(parent);
+
 	/* remove from sysfs if the caller did not do it */
 	if (kobj->state_in_sysfs) {
 		pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n",
@@ -645,6 +648,8 @@ static void kobject_cleanup(struct kobject *kobj)
 		t->release(kobj);
 	}
 
+	kobject_put(parent);
+
 	/* free name if we allocated it */
 	if (name) {
 		pr_debug("kobject: '%s': free name\n", name);
-- 
2.6.0.rc2.230.g3dd15c0

Re: [PATCH] kobject: Ensure child's resources get released before parent's resources

[Greg Kroah-Hartman]

On Mon, Dec 14, 2015 at 11:02:46AM -0800, Rajat Jain wrote:
> If the only remaining reference to a parent, is the one taken by
> the child (in kobject_add_internal()), then when the last
> reference to the child goes away, both child and its parents
> shall be released. However, currently the resources of parent
> get released first, followed by the child's resources:
> 
> kobject_cleanup(child)
>     ....
>     kobject_del(child)
>         ....
>         kobject_put(child->parent) -> results in parent's release()
>         ...
>     child->kobj_type->release() -> Child's release()
> 
> This is a problem because the child's release() method may still
> need to use parent resources or memory for its own cleanup. E.g.
> child may need parent pointer for dma_free_coherent() etc.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> Signed-off-by: Rajat Jain <rajatxjain@gmail.com>

Why are you listed twice here?

Where in the kernel is the parent being freed before the child that is
causing this issue to happen?  We should fix that root cause first...

thanks,

greg k-h

Re: [PATCH] kobject: Ensure child's resources get released before parent's resources

[Rajat Jain]

On Mon, Dec 14, 2015 at 1:40 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Dec 14, 2015 at 11:02:46AM -0800, Rajat Jain wrote:
>> If the only remaining reference to a parent, is the one taken by
>> the child (in kobject_add_internal()), then when the last
>> reference to the child goes away, both child and its parents
>> shall be released. However, currently the resources of parent
>> get released first, followed by the child's resources:
>>
>> kobject_cleanup(child)
>>     ....
>>     kobject_del(child)
>>         ....
>>         kobject_put(child->parent) -> results in parent's release()
>>         ...
>>     child->kobj_type->release() -> Child's release()
>>
>> This is a problem because the child's release() method may still
>> need to use parent resources or memory for its own cleanup. E.g.
>> child may need parent pointer for dma_free_coherent() etc.
>>
>> Signed-off-by: Rajat Jain <rajatja@google.com>
>> Signed-off-by: Rajat Jain <rajatxjain@gmail.com>
>
> Why are you listed twice here?

Ah, sorry, I'll remove that.

>
> Where in the kernel is the parent being freed before the child that is
> causing this issue to happen?  We should fix that root cause first...

Umm, are you saying that it is a bug to reach a scenario where all
references to a parent, except the ones made by the child, are gone?

Sorry, I should have given more context here. Here is the scenario
where I came across this situation, and I'd appreciate any suggestions
on how to better deal with this situation:

I have 2 modules (random names here):

user_interface.ko <--- pci_driver.ko

1) user_interface.ko
     - exports some interfaces (char driver etc) to the userspace,
     - allows low-level device drivers to register devices via some
API (user_interface_add() / user_interface_del())
     - Userspace can issue some transactions. Each transaction results
in a child kobject being attached to the device's kobject.
     - Low level drivers also provide a release() function that can
get called AFTER user_interface_del() if there are transactions
in-flight.
     - Low level drivers should allow operation of the device until
release() gets called.

2) Low level drivers such as pci_driver.ko:
    - attach to the actual physical devices (PCI device in this case)
    - create a custom device (that has an embedded "struct device")
and register this new custom device with the user_interface.ko.
    - also attaches a release() function to the device. This release()
would get called when all references to the device are dropped.
    - The entities holding the reference to the device are:
          * 1 reference by the pci_driver.ko itself (when it did
device_initialize())
          * 1 reference by the user_interface.ko (During user_interface_add())
          * 1 reference for each transaction in-flight (a child
kobject under the device)

3) Now, we want to allow removing (rmmod) the low level driver pci_driver.ko.
    - Before returning from PCI remove method, need to ensure that
release() has been called.
    - So we do call user_interface_del(dev) - drops the reference that
user_interface.ko was holding.
    - pci_driver.ko gives up its own reference so that release()
method can get called.
    - At this time, the device is just waiting for transactions
in-flight to get completed i.e only the child kobjects hold the
references.

When the last transaction gets completed, I end up in the situation
described in the patch commit log. I'd be very glad if you can provide
suggestions on how to achieve this or if there is anything I am
missing?

On a side note, my poor understanding of the device model came out to
that it does (or may be should) guarantee that all children are freed
before the parent is freed. Is that not the case?

Thanks,

Rajat

>
> thanks,
>
> greg k-h

Re: [PATCH] kobject: Ensure child's resources get released before parent's resources

[Rajat Jain]

[Fixed the linux-pci mailing list address]

On Mon, Dec 14, 2015 at 2:33 PM, Rajat Jain <rajatxjain@gmail.com> wrote:
> On Mon, Dec 14, 2015 at 1:40 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>> On Mon, Dec 14, 2015 at 11:02:46AM -0800, Rajat Jain wrote:
>>> If the only remaining reference to a parent, is the one taken by
>>> the child (in kobject_add_internal()), then when the last
>>> reference to the child goes away, both child and its parents
>>> shall be released. However, currently the resources of parent
>>> get released first, followed by the child's resources:
>>>
>>> kobject_cleanup(child)
>>>     ....
>>>     kobject_del(child)
>>>         ....
>>>         kobject_put(child->parent) -> results in parent's release()
>>>         ...
>>>     child->kobj_type->release() -> Child's release()
>>>
>>> This is a problem because the child's release() method may still
>>> need to use parent resources or memory for its own cleanup. E.g.
>>> child may need parent pointer for dma_free_coherent() etc.
>>>
>>> Signed-off-by: Rajat Jain <rajatja@google.com>
>>> Signed-off-by: Rajat Jain <rajatxjain@gmail.com>
>>
>> Why are you listed twice here?
>
> Ah, sorry, I'll remove that.
>
>>
>> Where in the kernel is the parent being freed before the child that is
>> causing this issue to happen?  We should fix that root cause first...
>
> Umm, are you saying that it is a bug to reach a scenario where all
> references to a parent, except the ones made by the child, are gone?
>
> Sorry, I should have given more context here. Here is the scenario
> where I came across this situation, and I'd appreciate any suggestions
> on how to better deal with this situation:
>
> I have 2 modules (random names here):
>
> user_interface.ko <--- pci_driver.ko
>
> 1) user_interface.ko
>      - exports some interfaces (char driver etc) to the userspace,
>      - allows low-level device drivers to register devices via some
> API (user_interface_add() / user_interface_del())
>      - Userspace can issue some transactions. Each transaction results
> in a child kobject being attached to the device's kobject.
>      - Low level drivers also provide a release() function that can
> get called AFTER user_interface_del() if there are transactions
> in-flight.
>      - Low level drivers should allow operation of the device until
> release() gets called.
>
> 2) Low level drivers such as pci_driver.ko:
>     - attach to the actual physical devices (PCI device in this case)
>     - create a custom device (that has an embedded "struct device")
> and register this new custom device with the user_interface.ko.
>     - also attaches a release() function to the device. This release()
> would get called when all references to the device are dropped.
>     - The entities holding the reference to the device are:
>           * 1 reference by the pci_driver.ko itself (when it did
> device_initialize())
>           * 1 reference by the user_interface.ko (During user_interface_add())
>           * 1 reference for each transaction in-flight (a child
> kobject under the device)
>
> 3) Now, we want to allow removing (rmmod) the low level driver pci_driver.ko.
>     - Before returning from PCI remove method, need to ensure that
> release() has been called.
>     - So we do call user_interface_del(dev) - drops the reference that
> user_interface.ko was holding.
>     - pci_driver.ko gives up its own reference so that release()
> method can get called.
>     - At this time, the device is just waiting for transactions
> in-flight to get completed i.e only the child kobjects hold the
> references.
>
> When the last transaction gets completed, I end up in the situation
> described in the patch commit log. I'd be very glad if you can provide
> suggestions on how to achieve this or if there is anything I am
> missing?
>
> On a side note, my poor understanding of the device model came out to
> that it does (or may be should) guarantee that all children are freed
> before the parent is freed. Is that not the case?
>
> Thanks,
>
> Rajat
>
>>
>> thanks,
>>
>> greg k-h

Re: [PATCH] kobject: Ensure child's resources get released before parent's resources

[Greg Kroah-Hartman]

On Mon, Dec 14, 2015 at 02:33:46PM -0800, Rajat Jain wrote:
> On Mon, Dec 14, 2015 at 1:40 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Dec 14, 2015 at 11:02:46AM -0800, Rajat Jain wrote:
> >> If the only remaining reference to a parent, is the one taken by
> >> the child (in kobject_add_internal()), then when the last
> >> reference to the child goes away, both child and its parents
> >> shall be released. However, currently the resources of parent
> >> get released first, followed by the child's resources:
> >>
> >> kobject_cleanup(child)
> >>     ....
> >>     kobject_del(child)
> >>         ....
> >>         kobject_put(child->parent) -> results in parent's release()
> >>         ...
> >>     child->kobj_type->release() -> Child's release()
> >>
> >> This is a problem because the child's release() method may still
> >> need to use parent resources or memory for its own cleanup. E.g.
> >> child may need parent pointer for dma_free_coherent() etc.
> >>
> >> Signed-off-by: Rajat Jain <rajatja@google.com>
> >> Signed-off-by: Rajat Jain <rajatxjain@gmail.com>
> >
> > Why are you listed twice here?
> 
> Ah, sorry, I'll remove that.
> 
> >
> > Where in the kernel is the parent being freed before the child that is
> > causing this issue to happen?  We should fix that root cause first...
> 
> Umm, are you saying that it is a bug to reach a scenario where all
> references to a parent, except the ones made by the child, are gone?
> 
> Sorry, I should have given more context here. Here is the scenario
> where I came across this situation, and I'd appreciate any suggestions
> on how to better deal with this situation:
> 
> I have 2 modules (random names here):
> 
> user_interface.ko <--- pci_driver.ko

Don't use random names, use real ones of existing drivers so we can see
what is happening here.

> 1) user_interface.ko
>      - exports some interfaces (char driver etc) to the userspace,

The specific type of interface is key here.

>      - allows low-level device drivers to register devices via some
> API (user_interface_add() / user_interface_del())

It better do so in a way that prevents itself from being unloaded...

>      - Userspace can issue some transactions. Each transaction results
> in a child kobject being attached to the device's kobject.

What?

What "child kobject"?  What "transaction" is being attached?  That's not
what kobjects are for.

Please post the code you are doing this with please.

>      - Low level drivers also provide a release() function that can
> get called AFTER user_interface_del() if there are transactions
> in-flight.

Then that's a stupid interface, fix it!  :)

>      - Low level drivers should allow operation of the device until
> release() gets called.

Nope, again, broken interface, fix it.

> 2) Low level drivers such as pci_driver.ko:
>     - attach to the actual physical devices (PCI device in this case)
>     - create a custom device (that has an embedded "struct device")
> and register this new custom device with the user_interface.ko.

That's odd, and not normal, you shouldn't be passing your device off to
anything else.  Where in the code does it do this?

>     - also attaches a release() function to the device. This release()
> would get called when all references to the device are dropped.

That release better be somewhere that is properly reference counted...

>     - The entities holding the reference to the device are:
>           * 1 reference by the pci_driver.ko itself (when it did
> device_initialize())
>           * 1 reference by the user_interface.ko (During user_interface_add())

Ick, no, broken interface.

>           * 1 reference for each transaction in-flight (a child
> kobject under the device)

Again, ick, no, broken interface, fix your code.  That's not how you
should design things.

> 3) Now, we want to allow removing (rmmod) the low level driver pci_driver.ko.

No you do not, don't do that.  End of story :)

Please post your example code, it's really designed poorly, we can fix
that up.

thanks,

greg k-h

[PATCH] kobject: Ensure child's resources get released before parent's resources

[Rajat Jain]

If the only remaining reference to a parent, is the one taken by
the child (in kobject_add_internal()), then when the last
reference to the child goes away, both child and its parents
shall be released. However, currently the resources of parent
get released first, followed by the child's resources:

kobject_cleanup(child)
    ....
    kobject_del(child)
        ....
        kobject_put(child->parent) -> results in parent's release()
        ...
    child->kobj_type->release() -> Child's release()

This is a problem because the child's release() method may still
need to use parent resources or memory for its own cleanup. E.g.
child may need parent pointer for dma_free_coherent() etc.

Signed-off-by: Rajat Jain <rajatja@google.com>
Signed-off-by: Rajat Jain <rajatxjain@gmail.com>
---
resending after an email address that bounced.

 lib/kobject.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/kobject.c b/lib/kobject.c
index 7cbccd2..8f325ac 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -616,6 +616,7 @@ static void kobject_cleanup(struct kobject *kobj)
 {
 	struct kobj_type *t = get_ktype(kobj);
 	const char *name = kobj->name;
+	struct kobject *parent = kobj->parent;
 
 	pr_debug("kobject: '%s' (%p): %s, parent %p\n",
 		 kobject_name(kobj), kobj, __func__, kobj->parent);
@@ -632,6 +633,8 @@ static void kobject_cleanup(struct kobject *kobj)
 		kobject_uevent(kobj, KOBJ_REMOVE);
 	}
 
+	kobject_get(parent);
+
 	/* remove from sysfs if the caller did not do it */
 	if (kobj->state_in_sysfs) {
 		pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n",
@@ -645,6 +648,8 @@ static void kobject_cleanup(struct kobject *kobj)
 		t->release(kobj);
 	}
 
+	kobject_put(parent);
+
 	/* free name if we allocated it */
 	if (name) {
 		pr_debug("kobject: '%s': free name\n", name);
-- 
2.6.0.rc2.230.g3dd15c0

[PATCH] kobject: Ensure child's resources get released before parent's resources

[Rajat Jain]

If the only remaining reference to a parent, is the one taken by
the child (in kobject_add_internal()), then when the last
reference to the child goes away, both child and its parents
shall be released. However, currently the resources of parent
get released first, followed by the child's resources:

kobject_cleanup(child)
    ....
    kobject_del(child)
        ....
        kobject_put(child->parent) -> results in parent's release()
        ...
    child->kobj_type->release() -> Child's release()

This is a problem because the child's release() method may still
need to use parent resources or memory for its own cleanup. E.g.
child may need parent pointer for dma_free_coherent() etc.

Signed-off-by: Rajat Jain <rajatja@google.com>
Signed-off-by: Rajat Jain <rajaxjain@gmail.com>
---
 lib/kobject.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/kobject.c b/lib/kobject.c
index 7cbccd2..8f325ac 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -616,6 +616,7 @@ static void kobject_cleanup(struct kobject *kobj)
 {
 	struct kobj_type *t = get_ktype(kobj);
 	const char *name = kobj->name;
+	struct kobject *parent = kobj->parent;
 
 	pr_debug("kobject: '%s' (%p): %s, parent %p\n",
 		 kobject_name(kobj), kobj, __func__, kobj->parent);
@@ -632,6 +633,8 @@ static void kobject_cleanup(struct kobject *kobj)
 		kobject_uevent(kobj, KOBJ_REMOVE);
 	}
 
+	kobject_get(parent);
+
 	/* remove from sysfs if the caller did not do it */
 	if (kobj->state_in_sysfs) {
 		pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n",
@@ -645,6 +648,8 @@ static void kobject_cleanup(struct kobject *kobj)
 		t->release(kobj);
 	}
 
+	kobject_put(parent);
+
 	/* free name if we allocated it */
 	if (name) {
 		pr_debug("kobject: '%s': free name\n", name);
-- 
2.6.0.rc2.230.g3dd15c0