[PATCH v1] kobject: make sure parent is not released before children

[PATCH v1] kobject: make sure parent is not released before children

[Brendan Higgins]

From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Previously, kobjects were released before the associated kobj_types;
this can cause a kobject deallocation to fail when the kobject has
children; an example of this is in software_node_unregister_nodes(); it
calls release on the parent before children meaning that children can be
released after the parent, which may be needed for removal.

So, take a reference to the parent before we delete a node to ensure
that the parent is not released before the children.

Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Fixes: 7589238a8cf3 ("Revert "software node: Simplify software_node_release() function"")
Link: https://lore.kernel.org/linux-kselftest/CAFd5g44s5NQvT8TG_x4rwbqoa7zWzkV0TX+ETZoQdOB7OwXCPQ@mail.gmail.com/T/#m71f37f3985f2abd7209c8ca8e0fa4edc45e171d6
Co-developed-by: Brendan Higgins <brendanhiggins@google.com>
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---

This patch is based on the diff written by Heikki linked above.

Heikki, can you either reply with a Signed-off-by? Otherwise, I can
resend with me as the author and I will list you as the Co-developed-by.

Sorry for all the CCs: I just want to make sure everyone who was a party
to the original bug sees this.

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

diff --git a/lib/kobject.c b/lib/kobject.c
index 83198cb37d8d..5921e2470b46 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -663,6 +663,7 @@ EXPORT_SYMBOL(kobject_get_unless_zero);
  */
 static void kobject_cleanup(struct kobject *kobj)
 {
+	struct kobject *parent = kobj->parent;
 	struct kobj_type *t = get_ktype(kobj);
 	const char *name = kobj->name;
 
@@ -680,6 +681,9 @@ static void kobject_cleanup(struct kobject *kobj)
 		kobject_uevent(kobj, KOBJ_REMOVE);
 	}
 
+	/* make sure the parent is not released before the (last) child */
+	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",
@@ -693,6 +697,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);

base-commit: 8632e9b5645bbc2331d21d892b0d6961c1a08429
-- 
2.26.0.110.g2183baf09c-goog

Re: [PATCH v1] kobject: make sure parent is not released before children

[Randy Dunlap]

On 4/14/20 1:42 PM, Brendan Higgins wrote:
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> Previously, kobjects were released before the associated kobj_types;
> this can cause a kobject deallocation to fail when the kobject has
> children; an example of this is in software_node_unregister_nodes(); it
> calls release on the parent before children meaning that children can be
> released after the parent, which may be needed for removal.
> 
> So, take a reference to the parent before we delete a node to ensure
> that the parent is not released before the children.
> 
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Fixes: 7589238a8cf3 ("Revert "software node: Simplify software_node_release() function"")
> Link: https://lore.kernel.org/linux-kselftest/CAFd5g44s5NQvT8TG_x4rwbqoa7zWzkV0TX+ETZoQdOB7OwXCPQ@mail.gmail.com/T/#m71f37f3985f2abd7209c8ca8e0fa4edc45e171d6
> Co-developed-by: Brendan Higgins <brendanhiggins@google.com>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>

Tested-by: Randy Dunlap <rdunlap@infradead.org>

Fixes the lib/test_printf.ko use-after-free on linux-next 20200410
that I reported last week.


> ---
> 
> This patch is based on the diff written by Heikki linked above.
> 
> Heikki, can you either reply with a Signed-off-by? Otherwise, I can
> resend with me as the author and I will list you as the Co-developed-by.
> 
> Sorry for all the CCs: I just want to make sure everyone who was a party
> to the original bug sees this.
> 
> ---
>  lib/kobject.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/kobject.c b/lib/kobject.c
> index 83198cb37d8d..5921e2470b46 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -663,6 +663,7 @@ EXPORT_SYMBOL(kobject_get_unless_zero);
>   */
>  static void kobject_cleanup(struct kobject *kobj)
>  {
> +	struct kobject *parent = kobj->parent;
>  	struct kobj_type *t = get_ktype(kobj);
>  	const char *name = kobj->name;
>  
> @@ -680,6 +681,9 @@ static void kobject_cleanup(struct kobject *kobj)
>  		kobject_uevent(kobj, KOBJ_REMOVE);
>  	}
>  
> +	/* make sure the parent is not released before the (last) child */
> +	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",
> @@ -693,6 +697,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);
> 
> base-commit: 8632e9b5645bbc2331d21d892b0d6961c1a08429
> 

Thanks.
-- 
~Randy

Re: [PATCH v1] kobject: make sure parent is not released before children

[Greg KH]

On Tue, Apr 14, 2020 at 01:42:40PM -0700, Brendan Higgins wrote:
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> Previously, kobjects were released before the associated kobj_types;
> this can cause a kobject deallocation to fail when the kobject has
> children; an example of this is in software_node_unregister_nodes(); it
> calls release on the parent before children meaning that children can be
> released after the parent, which may be needed for removal.

The simple solution for this is "don't do this" :)

> So, take a reference to the parent before we delete a node to ensure
> that the parent is not released before the children.
> 
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Fixes: 7589238a8cf3 ("Revert "software node: Simplify software_node_release() function"")
> Link: https://lore.kernel.org/linux-kselftest/CAFd5g44s5NQvT8TG_x4rwbqoa7zWzkV0TX+ETZoQdOB7OwXCPQ@mail.gmail.com/T/#m71f37f3985f2abd7209c8ca8e0fa4edc45e171d6
> Co-developed-by: Brendan Higgins <brendanhiggins@google.com>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---
> 
> This patch is based on the diff written by Heikki linked above.
> 
> Heikki, can you either reply with a Signed-off-by? Otherwise, I can
> resend with me as the author and I will list you as the Co-developed-by.
> 
> Sorry for all the CCs: I just want to make sure everyone who was a party
> to the original bug sees this.
> 
> ---
>  lib/kobject.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/kobject.c b/lib/kobject.c
> index 83198cb37d8d..5921e2470b46 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -663,6 +663,7 @@ EXPORT_SYMBOL(kobject_get_unless_zero);
>   */
>  static void kobject_cleanup(struct kobject *kobj)
>  {
> +	struct kobject *parent = kobj->parent;
>  	struct kobj_type *t = get_ktype(kobj);
>  	const char *name = kobj->name;
>  
> @@ -680,6 +681,9 @@ static void kobject_cleanup(struct kobject *kobj)
>  		kobject_uevent(kobj, KOBJ_REMOVE);
>  	}
>  
> +	/* make sure the parent is not released before the (last) child */
> +	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",
> @@ -693,6 +697,8 @@ static void kobject_cleanup(struct kobject *kobj)
>  		t->release(kobj);
>  	}
>  
> +	kobject_put(parent);
> +

No, please don't do this.

A child device should have always incremented the parent already if it
was correctly registered.  We have had this patch been proposed multiple
times over the years, and every time it was, we said no and went and
fixed the real issue which was with the user of the interface.

So, what code is causing this to happen?  What parent firmware device is
being removed that the code didn't walk the tree properly and remove the
children first?

thanks,

greg k-h

Re: [PATCH v1] kobject: make sure parent is not released before children

[Heikki Krogerus]

Hi Greg,

On Wed, Apr 15, 2020 at 08:11:54AM +0200, Greg KH wrote:
> > diff --git a/lib/kobject.c b/lib/kobject.c
> > index 83198cb37d8d..5921e2470b46 100644
> > --- a/lib/kobject.c
> > +++ b/lib/kobject.c
> > @@ -663,6 +663,7 @@ EXPORT_SYMBOL(kobject_get_unless_zero);
> >   */
> >  static void kobject_cleanup(struct kobject *kobj)
> >  {
> > +	struct kobject *parent = kobj->parent;
> >  	struct kobj_type *t = get_ktype(kobj);
> >  	const char *name = kobj->name;
> >  
> > @@ -680,6 +681,9 @@ static void kobject_cleanup(struct kobject *kobj)
> >  		kobject_uevent(kobj, KOBJ_REMOVE);
> >  	}
> >  
> > +	/* make sure the parent is not released before the (last) child */
> > +	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",
> > @@ -693,6 +697,8 @@ static void kobject_cleanup(struct kobject *kobj)
> >  		t->release(kobj);
> >  	}
> >  
> > +	kobject_put(parent);
> > +
> 
> No, please don't do this.
> 
> A child device should have always incremented the parent already if it
> was correctly registered.  We have had this patch been proposed multiple
> times over the years, and every time it was, we said no and went and
> fixed the real issue which was with the user of the interface.

The parent ref count is incremented by the child, that is not the
problem. The problem is that when that child is released, if it's the
last child of the parent, and there are no other users for the parent,
then the parent is actually released _before_ the child. And that
happens in the above function kobject_cleanup().

We can work around the problem by taking a reference to the parent
separately, but we have to do that everywhere separately (which I
guess is exactly what has been done so far). That workaroud still does
not really fix the core problem. The core problem is still that
lib/kboject.c is allowing the parent kobject to be released before the
child kobject, and that quite simply should not be allowed to happen.

I don't have a problem if you want to have a better solution for this,
but the solution really can't anymore be that we are always expected
to separately increment the parent's ref count with every type of
kobject.


thanks,

-- 
heikki

Re: [PATCH v1] kobject: make sure parent is not released before children

[Rafael J. Wysocki]

On Wed, Apr 15, 2020 at 10:47 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi Greg,
>
> On Wed, Apr 15, 2020 at 08:11:54AM +0200, Greg KH wrote:
> > > diff --git a/lib/kobject.c b/lib/kobject.c
> > > index 83198cb37d8d..5921e2470b46 100644
> > > --- a/lib/kobject.c
> > > +++ b/lib/kobject.c
> > > @@ -663,6 +663,7 @@ EXPORT_SYMBOL(kobject_get_unless_zero);
> > >   */
> > >  static void kobject_cleanup(struct kobject *kobj)
> > >  {
> > > +   struct kobject *parent = kobj->parent;
> > >     struct kobj_type *t = get_ktype(kobj);
> > >     const char *name = kobj->name;
> > >
> > > @@ -680,6 +681,9 @@ static void kobject_cleanup(struct kobject *kobj)
> > >             kobject_uevent(kobj, KOBJ_REMOVE);
> > >     }
> > >
> > > +   /* make sure the parent is not released before the (last) child */
> > > +   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",
> > > @@ -693,6 +697,8 @@ static void kobject_cleanup(struct kobject *kobj)
> > >             t->release(kobj);
> > >     }
> > >
> > > +   kobject_put(parent);
> > > +
> >
> > No, please don't do this.
> >
> > A child device should have always incremented the parent already if it
> > was correctly registered.  We have had this patch been proposed multiple
> > times over the years, and every time it was, we said no and went and
> > fixed the real issue which was with the user of the interface.
>
> The parent ref count is incremented by the child, that is not the
> problem. The problem is that when that child is released, if it's the
> last child of the parent, and there are no other users for the parent,
> then the parent is actually released _before_ the child. And that
> happens in the above function kobject_cleanup().

In fact, it happens in kobject_del() invoked by kobject_cleanup() AFAICS.

So it appears incorrect to use kobject_del() as is in the latter.

> We can work around the problem by taking a reference to the parent
> separately, but we have to do that everywhere separately (which I
> guess is exactly what has been done so far). That workaroud still does
> not really fix the core problem. The core problem is still that
> lib/kboject.c is allowing the parent kobject to be released before the
> child kobject, and that quite simply should not be allowed to happen.
>
> I don't have a problem if you want to have a better solution for this,
> but the solution really can't anymore be that we are always expected
> to separately increment the parent's ref count with every type of
> kobject.

An alternative might be to define something like __kobject_del() doing
everything that kobject_del() does *without* the
kobject_put(kobj->parent).

Then, kobject_del() could be defined as something like (pseudocode):

kobject_del(kobj)
{
    kobject *perent = kobj->parent;

    __kobject_del(kobj);
    kobject_put(parent);
}

and kobject_cleanup() could call __kobject_del() instead of
kobject_del() and then do the last kobject_put(parent) when it is done
with the child.

Would that work?

Re: [PATCH v1] kobject: make sure parent is not released before children

[Heikki Krogerus]

On Wed, Apr 15, 2020 at 11:21:03AM +0200, Rafael J. Wysocki wrote:
> On Wed, Apr 15, 2020 at 10:47 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > Hi Greg,
> >
> > On Wed, Apr 15, 2020 at 08:11:54AM +0200, Greg KH wrote:
> > > > diff --git a/lib/kobject.c b/lib/kobject.c
> > > > index 83198cb37d8d..5921e2470b46 100644
> > > > --- a/lib/kobject.c
> > > > +++ b/lib/kobject.c
> > > > @@ -663,6 +663,7 @@ EXPORT_SYMBOL(kobject_get_unless_zero);
> > > >   */
> > > >  static void kobject_cleanup(struct kobject *kobj)
> > > >  {
> > > > +   struct kobject *parent = kobj->parent;
> > > >     struct kobj_type *t = get_ktype(kobj);
> > > >     const char *name = kobj->name;
> > > >
> > > > @@ -680,6 +681,9 @@ static void kobject_cleanup(struct kobject *kobj)
> > > >             kobject_uevent(kobj, KOBJ_REMOVE);
> > > >     }
> > > >
> > > > +   /* make sure the parent is not released before the (last) child */
> > > > +   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",
> > > > @@ -693,6 +697,8 @@ static void kobject_cleanup(struct kobject *kobj)
> > > >             t->release(kobj);
> > > >     }
> > > >
> > > > +   kobject_put(parent);
> > > > +
> > >
> > > No, please don't do this.
> > >
> > > A child device should have always incremented the parent already if it
> > > was correctly registered.  We have had this patch been proposed multiple
> > > times over the years, and every time it was, we said no and went and
> > > fixed the real issue which was with the user of the interface.
> >
> > The parent ref count is incremented by the child, that is not the
> > problem. The problem is that when that child is released, if it's the
> > last child of the parent, and there are no other users for the parent,
> > then the parent is actually released _before_ the child. And that
> > happens in the above function kobject_cleanup().
> 
> In fact, it happens in kobject_del() invoked by kobject_cleanup() AFAICS.
> 
> So it appears incorrect to use kobject_del() as is in the latter.
> 
> > We can work around the problem by taking a reference to the parent
> > separately, but we have to do that everywhere separately (which I
> > guess is exactly what has been done so far). That workaroud still does
> > not really fix the core problem. The core problem is still that
> > lib/kboject.c is allowing the parent kobject to be released before the
> > child kobject, and that quite simply should not be allowed to happen.
> >
> > I don't have a problem if you want to have a better solution for this,
> > but the solution really can't anymore be that we are always expected
> > to separately increment the parent's ref count with every type of
> > kobject.
> 
> An alternative might be to define something like __kobject_del() doing
> everything that kobject_del() does *without* the
> kobject_put(kobj->parent).
> 
> Then, kobject_del() could be defined as something like (pseudocode):
> 
> kobject_del(kobj)
> {
>     kobject *perent = kobj->parent;
> 
>     __kobject_del(kobj);
>     kobject_put(parent);
> }
> 
> and kobject_cleanup() could call __kobject_del() instead of
> kobject_del() and then do the last kobject_put(parent) when it is done
> with the child.
> 
> Would that work?

I think so. Greg, what do you think?

thanks,

-- 
heikki

Re: [PATCH v1] kobject: make sure parent is not released before children

[Greg KH]

On Wed, Apr 15, 2020 at 04:10:18PM +0300, Heikki Krogerus wrote:
> On Wed, Apr 15, 2020 at 11:21:03AM +0200, Rafael J. Wysocki wrote:
> > On Wed, Apr 15, 2020 at 10:47 AM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > Hi Greg,
> > >
> > > On Wed, Apr 15, 2020 at 08:11:54AM +0200, Greg KH wrote:
> > > > > diff --git a/lib/kobject.c b/lib/kobject.c
> > > > > index 83198cb37d8d..5921e2470b46 100644
> > > > > --- a/lib/kobject.c
> > > > > +++ b/lib/kobject.c
> > > > > @@ -663,6 +663,7 @@ EXPORT_SYMBOL(kobject_get_unless_zero);
> > > > >   */
> > > > >  static void kobject_cleanup(struct kobject *kobj)
> > > > >  {
> > > > > +   struct kobject *parent = kobj->parent;
> > > > >     struct kobj_type *t = get_ktype(kobj);
> > > > >     const char *name = kobj->name;
> > > > >
> > > > > @@ -680,6 +681,9 @@ static void kobject_cleanup(struct kobject *kobj)
> > > > >             kobject_uevent(kobj, KOBJ_REMOVE);
> > > > >     }
> > > > >
> > > > > +   /* make sure the parent is not released before the (last) child */
> > > > > +   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",
> > > > > @@ -693,6 +697,8 @@ static void kobject_cleanup(struct kobject *kobj)
> > > > >             t->release(kobj);
> > > > >     }
> > > > >
> > > > > +   kobject_put(parent);
> > > > > +
> > > >
> > > > No, please don't do this.
> > > >
> > > > A child device should have always incremented the parent already if it
> > > > was correctly registered.  We have had this patch been proposed multiple
> > > > times over the years, and every time it was, we said no and went and
> > > > fixed the real issue which was with the user of the interface.
> > >
> > > The parent ref count is incremented by the child, that is not the
> > > problem. The problem is that when that child is released, if it's the
> > > last child of the parent, and there are no other users for the parent,
> > > then the parent is actually released _before_ the child. And that
> > > happens in the above function kobject_cleanup().
> > 
> > In fact, it happens in kobject_del() invoked by kobject_cleanup() AFAICS.
> > 
> > So it appears incorrect to use kobject_del() as is in the latter.
> > 
> > > We can work around the problem by taking a reference to the parent
> > > separately, but we have to do that everywhere separately (which I
> > > guess is exactly what has been done so far). That workaroud still does
> > > not really fix the core problem. The core problem is still that
> > > lib/kboject.c is allowing the parent kobject to be released before the
> > > child kobject, and that quite simply should not be allowed to happen.
> > >
> > > I don't have a problem if you want to have a better solution for this,
> > > but the solution really can't anymore be that we are always expected
> > > to separately increment the parent's ref count with every type of
> > > kobject.
> > 
> > An alternative might be to define something like __kobject_del() doing
> > everything that kobject_del() does *without* the
> > kobject_put(kobj->parent).
> > 
> > Then, kobject_del() could be defined as something like (pseudocode):
> > 
> > kobject_del(kobj)
> > {
> >     kobject *perent = kobj->parent;
> > 
> >     __kobject_del(kobj);
> >     kobject_put(parent);
> > }
> > 
> > and kobject_cleanup() could call __kobject_del() instead of
> > kobject_del() and then do the last kobject_put(parent) when it is done
> > with the child.
> > 
> > Would that work?
> 
> I think so. Greg, what do you think?

Hm, maybe.  Can someone test it out with the reproducer?

thanks,

greg k-h

Re: [PATCH v1] kobject: make sure parent is not released before children

[Heikki Krogerus]

Hi,

> > > An alternative might be to define something like __kobject_del() doing
> > > everything that kobject_del() does *without* the
> > > kobject_put(kobj->parent).
> > > 
> > > Then, kobject_del() could be defined as something like (pseudocode):
> > > 
> > > kobject_del(kobj)
> > > {
> > >     kobject *perent = kobj->parent;
> > > 
> > >     __kobject_del(kobj);
> > >     kobject_put(parent);
> > > }
> > > 
> > > and kobject_cleanup() could call __kobject_del() instead of
> > > kobject_del() and then do the last kobject_put(parent) when it is done
> > > with the child.
> > > 
> > > Would that work?
> > 
> > I think so. Greg, what do you think?
> 
> Hm, maybe.  Can someone test it out with the reproducer?

Brendan, or Randy! Can you guys test Rafael's proposal? I think it
would look like this:

diff --git a/lib/kobject.c b/lib/kobject.c
index 83198cb37d8d..2bd631460e18 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -599,14 +599,7 @@ int kobject_move(struct kobject *kobj, struct kobject *new_parent)
 }
 EXPORT_SYMBOL_GPL(kobject_move);
 
-/**
- * kobject_del() - Unlink kobject from hierarchy.
- * @kobj: object.
- *
- * This is the function that should be called to delete an object
- * successfully added via kobject_add().
- */
-void kobject_del(struct kobject *kobj)
+static void __kobject_del(struct kobject *kobj)
 {
        struct kernfs_node *sd;
        const struct kobj_type *ktype;
@@ -625,9 +618,23 @@ void kobject_del(struct kobject *kobj)
 
        kobj->state_in_sysfs = 0;
        kobj_kset_leave(kobj);
-       kobject_put(kobj->parent);
        kobj->parent = NULL;
 }
+
+/**
+ * kobject_del() - Unlink kobject from hierarchy.
+ * @kobj: object.
+ *
+ * This is the function that should be called to delete an object
+ * successfully added via kobject_add().
+ */
+void kobject_del(struct kobject *kobj)
+{
+       struct kobject *parent = kobj->parent;
+
+       __kobject_del(kobj);
+       kobject_put(parent);
+}
 EXPORT_SYMBOL(kobject_del);
 
 /**
@@ -663,6 +670,7 @@ EXPORT_SYMBOL(kobject_get_unless_zero);
  */
 static void kobject_cleanup(struct kobject *kobj)
 {
+       struct kobject *parent = kobj->parent;
        struct kobj_type *t = get_ktype(kobj);
        const char *name = kobj->name;
 
@@ -684,7 +692,7 @@ static void kobject_cleanup(struct kobject *kobj)
        if (kobj->state_in_sysfs) {
                pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n",
                         kobject_name(kobj), kobj);
-               kobject_del(kobj);
+               __kobject_del(kobj);
        }
 
        if (t && t->release) {
@@ -698,6 +706,8 @@ static void kobject_cleanup(struct kobject *kobj)
                pr_debug("kobject: '%s': free name\n", name);
                kfree_const(name);
        }
+
+       kobject_put(parent);
 }

 #ifdef CONFIG_DEBUG_KOBJECT_RELEASE


thanks,

-- 
heikki

Re: [PATCH v1] kobject: make sure parent is not released before children

[Randy Dunlap]

On 4/17/20 4:39 AM, Heikki Krogerus wrote:
> Hi,
> 
>>>> An alternative might be to define something like __kobject_del() doing
>>>> everything that kobject_del() does *without* the
>>>> kobject_put(kobj->parent).
>>>>
>>>> Then, kobject_del() could be defined as something like (pseudocode):
>>>>
>>>> kobject_del(kobj)
>>>> {
>>>>     kobject *perent = kobj->parent;
>>>>
>>>>     __kobject_del(kobj);
>>>>     kobject_put(parent);
>>>> }
>>>>
>>>> and kobject_cleanup() could call __kobject_del() instead of
>>>> kobject_del() and then do the last kobject_put(parent) when it is done
>>>> with the child.
>>>>
>>>> Would that work?
>>>
>>> I think so. Greg, what do you think?
>>
>> Hm, maybe.  Can someone test it out with the reproducer?
> 
> Brendan, or Randy! Can you guys test Rafael's proposal? I think it
> would look like this:

patch is whitespace-damaged. did you copy-paste it from a screen?


Anyway, it works for me. I loaded & unloaded test_printf.ko 5 times
without a problem.

Thanks.

> diff --git a/lib/kobject.c b/lib/kobject.c
> index 83198cb37d8d..2bd631460e18 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -599,14 +599,7 @@ int kobject_move(struct kobject *kobj, struct kobject *new_parent)
>  }
>  EXPORT_SYMBOL_GPL(kobject_move);
>  
> -/**
> - * kobject_del() - Unlink kobject from hierarchy.
> - * @kobj: object.
> - *
> - * This is the function that should be called to delete an object
> - * successfully added via kobject_add().
> - */
> -void kobject_del(struct kobject *kobj)
> +static void __kobject_del(struct kobject *kobj)
>  {
>         struct kernfs_node *sd;
>         const struct kobj_type *ktype;
> @@ -625,9 +618,23 @@ void kobject_del(struct kobject *kobj)
>  
>         kobj->state_in_sysfs = 0;
>         kobj_kset_leave(kobj);
> -       kobject_put(kobj->parent);
>         kobj->parent = NULL;
>  }
> +
> +/**
> + * kobject_del() - Unlink kobject from hierarchy.
> + * @kobj: object.
> + *
> + * This is the function that should be called to delete an object
> + * successfully added via kobject_add().
> + */
> +void kobject_del(struct kobject *kobj)
> +{
> +       struct kobject *parent = kobj->parent;
> +
> +       __kobject_del(kobj);
> +       kobject_put(parent);
> +}
>  EXPORT_SYMBOL(kobject_del);
>  
>  /**
> @@ -663,6 +670,7 @@ EXPORT_SYMBOL(kobject_get_unless_zero);
>   */
>  static void kobject_cleanup(struct kobject *kobj)
>  {
> +       struct kobject *parent = kobj->parent;
>         struct kobj_type *t = get_ktype(kobj);
>         const char *name = kobj->name;
>  
> @@ -684,7 +692,7 @@ static void kobject_cleanup(struct kobject *kobj)
>         if (kobj->state_in_sysfs) {
>                 pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n",
>                          kobject_name(kobj), kobj);
> -               kobject_del(kobj);
> +               __kobject_del(kobj);
>         }
>  
>         if (t && t->release) {
> @@ -698,6 +706,8 @@ static void kobject_cleanup(struct kobject *kobj)
>                 pr_debug("kobject: '%s': free name\n", name);
>                 kfree_const(name);
>         }
> +
> +       kobject_put(parent);
>  }
> 
>  #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
> 
> 
> thanks,
> 


-- 
~Randy
Reported-by: Randy Dunlap <rdunlap@infradead.org>

Re: [PATCH v1] kobject: make sure parent is not released before children

[Brendan Higgins]

On Fri, Apr 17, 2020 at 9:08 AM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> On 4/17/20 4:39 AM, Heikki Krogerus wrote:
> > Hi,
> >
> >>>> An alternative might be to define something like __kobject_del() doing
> >>>> everything that kobject_del() does *without* the
> >>>> kobject_put(kobj->parent).
> >>>>
> >>>> Then, kobject_del() could be defined as something like (pseudocode):
> >>>>
> >>>> kobject_del(kobj)
> >>>> {
> >>>>     kobject *perent = kobj->parent;
> >>>>
> >>>>     __kobject_del(kobj);
> >>>>     kobject_put(parent);
> >>>> }
> >>>>
> >>>> and kobject_cleanup() could call __kobject_del() instead of
> >>>> kobject_del() and then do the last kobject_put(parent) when it is done
> >>>> with the child.
> >>>>
> >>>> Would that work?
> >>>
> >>> I think so. Greg, what do you think?
> >>
> >> Hm, maybe.  Can someone test it out with the reproducer?
> >
> > Brendan, or Randy! Can you guys test Rafael's proposal? I think it
> > would look like this:
>
> patch is whitespace-damaged. did you copy-paste it from a screen?
>
>
> Anyway, it works for me. I loaded & unloaded test_printf.ko 5 times
> without a problem.

I just tried it as well. I also noticed some whitespace corruption,
but it otherwise worked against the KUnit reproducer I wrote:

https://lore.kernel.org/patchwork/patch/1224002/

Thanks!

> Thanks.
>
> > diff --git a/lib/kobject.c b/lib/kobject.c
> > index 83198cb37d8d..2bd631460e18 100644
> > --- a/lib/kobject.c
> > +++ b/lib/kobject.c
> > @@ -599,14 +599,7 @@ int kobject_move(struct kobject *kobj, struct kobject *new_parent)
> >  }
> >  EXPORT_SYMBOL_GPL(kobject_move);
> >
> > -/**
> > - * kobject_del() - Unlink kobject from hierarchy.
> > - * @kobj: object.
> > - *
> > - * This is the function that should be called to delete an object
> > - * successfully added via kobject_add().
> > - */
> > -void kobject_del(struct kobject *kobj)
> > +static void __kobject_del(struct kobject *kobj)
> >  {
> >         struct kernfs_node *sd;
> >         const struct kobj_type *ktype;
> > @@ -625,9 +618,23 @@ void kobject_del(struct kobject *kobj)
> >
> >         kobj->state_in_sysfs = 0;
> >         kobj_kset_leave(kobj);
> > -       kobject_put(kobj->parent);
> >         kobj->parent = NULL;
> >  }
> > +
> > +/**
> > + * kobject_del() - Unlink kobject from hierarchy.
> > + * @kobj: object.
> > + *
> > + * This is the function that should be called to delete an object
> > + * successfully added via kobject_add().
> > + */
> > +void kobject_del(struct kobject *kobj)
> > +{
> > +       struct kobject *parent = kobj->parent;
> > +
> > +       __kobject_del(kobj);
> > +       kobject_put(parent);
> > +}
> >  EXPORT_SYMBOL(kobject_del);
> >
> >  /**
> > @@ -663,6 +670,7 @@ EXPORT_SYMBOL(kobject_get_unless_zero);
> >   */
> >  static void kobject_cleanup(struct kobject *kobj)
> >  {
> > +       struct kobject *parent = kobj->parent;
> >         struct kobj_type *t = get_ktype(kobj);
> >         const char *name = kobj->name;
> >
> > @@ -684,7 +692,7 @@ static void kobject_cleanup(struct kobject *kobj)
> >         if (kobj->state_in_sysfs) {
> >                 pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n",
> >                          kobject_name(kobj), kobj);
> > -               kobject_del(kobj);
> > +               __kobject_del(kobj);
> >         }
> >
> >         if (t && t->release) {
> > @@ -698,6 +706,8 @@ static void kobject_cleanup(struct kobject *kobj)
> >                 pr_debug("kobject: '%s': free name\n", name);
> >                 kfree_const(name);
> >         }
> > +
> > +       kobject_put(parent);
> >  }
> >
> >  #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
> >
> >
> > thanks,
> >
>
>
> --
> ~Randy
> Reported-by: Randy Dunlap <rdunlap@infradead.org>

Re: [PATCH v1] kobject: make sure parent is not released before children

[Greg KH]

On Wed, Apr 15, 2020 at 11:46:53AM +0300, Heikki Krogerus wrote:
> Hi Greg,
> 
> On Wed, Apr 15, 2020 at 08:11:54AM +0200, Greg KH wrote:
> > > diff --git a/lib/kobject.c b/lib/kobject.c
> > > index 83198cb37d8d..5921e2470b46 100644
> > > --- a/lib/kobject.c
> > > +++ b/lib/kobject.c
> > > @@ -663,6 +663,7 @@ EXPORT_SYMBOL(kobject_get_unless_zero);
> > >   */
> > >  static void kobject_cleanup(struct kobject *kobj)
> > >  {
> > > +	struct kobject *parent = kobj->parent;
> > >  	struct kobj_type *t = get_ktype(kobj);
> > >  	const char *name = kobj->name;
> > >  
> > > @@ -680,6 +681,9 @@ static void kobject_cleanup(struct kobject *kobj)
> > >  		kobject_uevent(kobj, KOBJ_REMOVE);
> > >  	}
> > >  
> > > +	/* make sure the parent is not released before the (last) child */
> > > +	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",
> > > @@ -693,6 +697,8 @@ static void kobject_cleanup(struct kobject *kobj)
> > >  		t->release(kobj);
> > >  	}
> > >  
> > > +	kobject_put(parent);
> > > +
> > 
> > No, please don't do this.
> > 
> > A child device should have always incremented the parent already if it
> > was correctly registered.  We have had this patch been proposed multiple
> > times over the years, and every time it was, we said no and went and
> > fixed the real issue which was with the user of the interface.
> 
> The parent ref count is incremented by the child, that is not the
> problem. The problem is that when that child is released, if it's the
> last child of the parent, and there are no other users for the parent,
> then the parent is actually released _before_ the child. And that
> happens in the above function kobject_cleanup().
> 
> We can work around the problem by taking a reference to the parent
> separately, but we have to do that everywhere separately (which I
> guess is exactly what has been done so far). That workaroud still does
> not really fix the core problem. The core problem is still that
> lib/kboject.c is allowing the parent kobject to be released before the
> child kobject, and that quite simply should not be allowed to happen.
> 
> I don't have a problem if you want to have a better solution for this,
> but the solution really can't anymore be that we are always expected
> to separately increment the parent's ref count with every type of
> kobject.

Why is this suddenly showing up as a issue and it hasn't ever before?
Because of that I would argue that the problem is not in the kobject
core, but the use of it.

When a child object is created, it should have already incremented the
parent reference count before it is registered, to ensure that the
parent does not go away before the child is finished being registered.
Then, when the child is removed, it decrements the reference count of
the parent as it "knows" it is done with that pointer.

So perhaps this new swnode code is just wrong?  Dealing with "raw"
kobjects is hard, and not generally recommended due to issues like this.
When you use the driver core, all of this logic is already handled for
you...

And I just looked at the code swnode_register() needs to increment the
parent kobject's reference, as it is saving a pointer to it away to be
used later.  That's just basic reference-counted-pointer logic here,
please fix the issue there.

thanks,

greg k-h

Re: [PATCH v1] kobject: make sure parent is not released before children

[Heikki Krogerus]

On Wed, Apr 15, 2020 at 11:21:15AM +0200, Greg KH wrote:
> On Wed, Apr 15, 2020 at 11:46:53AM +0300, Heikki Krogerus wrote:
> > Hi Greg,
> > 
> > On Wed, Apr 15, 2020 at 08:11:54AM +0200, Greg KH wrote:
> > > > diff --git a/lib/kobject.c b/lib/kobject.c
> > > > index 83198cb37d8d..5921e2470b46 100644
> > > > --- a/lib/kobject.c
> > > > +++ b/lib/kobject.c
> > > > @@ -663,6 +663,7 @@ EXPORT_SYMBOL(kobject_get_unless_zero);
> > > >   */
> > > >  static void kobject_cleanup(struct kobject *kobj)
> > > >  {
> > > > +	struct kobject *parent = kobj->parent;
> > > >  	struct kobj_type *t = get_ktype(kobj);
> > > >  	const char *name = kobj->name;
> > > >  
> > > > @@ -680,6 +681,9 @@ static void kobject_cleanup(struct kobject *kobj)
> > > >  		kobject_uevent(kobj, KOBJ_REMOVE);
> > > >  	}
> > > >  
> > > > +	/* make sure the parent is not released before the (last) child */
> > > > +	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",
> > > > @@ -693,6 +697,8 @@ static void kobject_cleanup(struct kobject *kobj)
> > > >  		t->release(kobj);
> > > >  	}
> > > >  
> > > > +	kobject_put(parent);
> > > > +
> > > 
> > > No, please don't do this.
> > > 
> > > A child device should have always incremented the parent already if it
> > > was correctly registered.  We have had this patch been proposed multiple
> > > times over the years, and every time it was, we said no and went and
> > > fixed the real issue which was with the user of the interface.
> > 
> > The parent ref count is incremented by the child, that is not the
> > problem. The problem is that when that child is released, if it's the
> > last child of the parent, and there are no other users for the parent,
> > then the parent is actually released _before_ the child. And that
> > happens in the above function kobject_cleanup().
> > 
> > We can work around the problem by taking a reference to the parent
> > separately, but we have to do that everywhere separately (which I
> > guess is exactly what has been done so far). That workaroud still does
> > not really fix the core problem. The core problem is still that
> > lib/kboject.c is allowing the parent kobject to be released before the
> > child kobject, and that quite simply should not be allowed to happen.
> > 
> > I don't have a problem if you want to have a better solution for this,
> > but the solution really can't anymore be that we are always expected
> > to separately increment the parent's ref count with every type of
> > kobject.
> 
> Why is this suddenly showing up as a issue and it hasn't ever before?
> Because of that I would argue that the problem is not in the kobject
> core, but the use of it.

The problem has showed up before, and you pointed that out yourself in
your previous mail.

The thing is that we are going to continue to have this issue over and
over again until the core issue is fixed.

> When a child object is created, it should have already incremented the
> parent reference count before it is registered, to ensure that the
> parent does not go away before the child is finished being registered.
> Then, when the child is removed, it decrements the reference count of
> the parent as it "knows" it is done with that pointer.

Please note that when you add a kobject, its parent ref count gets
incremented (line 240 in lib/kobject.c). Are you saying here that that
is actually not necessary, as the code that registers the new (child)
kobject should _always_ be responsible of holding a reference to the
parent? Because if that is the case, then we should actually cleanup
lib/kboject.c and remove all parent reference handling. We obviously
need to fix the documentation as well in this case.

> So perhaps this new swnode code is just wrong?  Dealing with "raw"
> kobjects is hard, and not generally recommended due to issues like this.
> When you use the driver core, all of this logic is already handled for
> you...
> 
> And I just looked at the code swnode_register() needs to increment the
> parent kobject's reference, as it is saving a pointer to it away to be
> used later.  That's just basic reference-counted-pointer logic here,
> please fix the issue there.

That is fair, however, it still does not fix the core problem.

Even if we did not need to save the pointer to the parent, we would
still need to increment the parents reference count, because we have
to prevent the parent from being released before us.

There are two ways we can fix the situation:

1) We can make it clear that lib/kboject.c does not take any
   responsibility over the reference counting of the parents, and
   remove all the parent reference handling from it.

2) We fix this by guaranteeing that the parent kboject can not be
   released before its children.

thanks,

-- 
heikki

Re: [PATCH v1] kobject: make sure parent is not released before children

[Greg KH]

On Wed, Apr 15, 2020 at 02:25:03PM +0300, Heikki Krogerus wrote:
> On Wed, Apr 15, 2020 at 11:21:15AM +0200, Greg KH wrote:
> > On Wed, Apr 15, 2020 at 11:46:53AM +0300, Heikki Krogerus wrote:
> > > Hi Greg,
> > > 
> > > On Wed, Apr 15, 2020 at 08:11:54AM +0200, Greg KH wrote:
> > > > > diff --git a/lib/kobject.c b/lib/kobject.c
> > > > > index 83198cb37d8d..5921e2470b46 100644
> > > > > --- a/lib/kobject.c
> > > > > +++ b/lib/kobject.c
> > > > > @@ -663,6 +663,7 @@ EXPORT_SYMBOL(kobject_get_unless_zero);
> > > > >   */
> > > > >  static void kobject_cleanup(struct kobject *kobj)
> > > > >  {
> > > > > +	struct kobject *parent = kobj->parent;
> > > > >  	struct kobj_type *t = get_ktype(kobj);
> > > > >  	const char *name = kobj->name;
> > > > >  
> > > > > @@ -680,6 +681,9 @@ static void kobject_cleanup(struct kobject *kobj)
> > > > >  		kobject_uevent(kobj, KOBJ_REMOVE);
> > > > >  	}
> > > > >  
> > > > > +	/* make sure the parent is not released before the (last) child */
> > > > > +	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",
> > > > > @@ -693,6 +697,8 @@ static void kobject_cleanup(struct kobject *kobj)
> > > > >  		t->release(kobj);
> > > > >  	}
> > > > >  
> > > > > +	kobject_put(parent);
> > > > > +
> > > > 
> > > > No, please don't do this.
> > > > 
> > > > A child device should have always incremented the parent already if it
> > > > was correctly registered.  We have had this patch been proposed multiple
> > > > times over the years, and every time it was, we said no and went and
> > > > fixed the real issue which was with the user of the interface.
> > > 
> > > The parent ref count is incremented by the child, that is not the
> > > problem. The problem is that when that child is released, if it's the
> > > last child of the parent, and there are no other users for the parent,
> > > then the parent is actually released _before_ the child. And that
> > > happens in the above function kobject_cleanup().
> > > 
> > > We can work around the problem by taking a reference to the parent
> > > separately, but we have to do that everywhere separately (which I
> > > guess is exactly what has been done so far). That workaroud still does
> > > not really fix the core problem. The core problem is still that
> > > lib/kboject.c is allowing the parent kobject to be released before the
> > > child kobject, and that quite simply should not be allowed to happen.
> > > 
> > > I don't have a problem if you want to have a better solution for this,
> > > but the solution really can't anymore be that we are always expected
> > > to separately increment the parent's ref count with every type of
> > > kobject.
> > 
> > Why is this suddenly showing up as a issue and it hasn't ever before?
> > Because of that I would argue that the problem is not in the kobject
> > core, but the use of it.
> 
> The problem has showed up before, and you pointed that out yourself in
> your previous mail.
> 
> The thing is that we are going to continue to have this issue over and
> over again until the core issue is fixed.
> 
> > When a child object is created, it should have already incremented the
> > parent reference count before it is registered, to ensure that the
> > parent does not go away before the child is finished being registered.
> > Then, when the child is removed, it decrements the reference count of
> > the parent as it "knows" it is done with that pointer.
> 
> Please note that when you add a kobject, its parent ref count gets
> incremented (line 240 in lib/kobject.c). Are you saying here that that
> is actually not necessary, as the code that registers the new (child)
> kobject should _always_ be responsible of holding a reference to the
> parent? Because if that is the case, then we should actually cleanup
> lib/kboject.c and remove all parent reference handling. We obviously
> need to fix the documentation as well in this case.

Hm, no, I think the code is all correct as-is, with one exception that I
think you all are triggering now.

kobject_add() ends up incrementing the parent and then will properly
drop it when kobject_del() is called.

The issue is that kobject_cleanup() is only called when the memory of
the object needs to go away.  It has long been removed from sysfs
(hopefully, if not that's already handled in the call).

But, the parent could be gone here, as you all are seeing, so I think
the line:

       pr_debug("kobject: '%s' (%p): %s, parent %p\n",
                 kobject_name(kobj), kobj, __func__, kobj->parent);

Is causing the problem, as kobj->parent could be gone.  Well, it's not a
problem as that's just a pointer (odd call that one, should be fixed up
one day...)

But later on we try to call kobject_uevent() and that looks for the
parent in the fill_kobj_path() call, right?

Can someone provide a stack trace for the issue?  The patch here just
papers over the race condition, should we move the kobject_put() of the
parent into this function instead?

> > So perhaps this new swnode code is just wrong?  Dealing with "raw"
> > kobjects is hard, and not generally recommended due to issues like this.
> > When you use the driver core, all of this logic is already handled for
> > you...
> > 
> > And I just looked at the code swnode_register() needs to increment the
> > parent kobject's reference, as it is saving a pointer to it away to be
> > used later.  That's just basic reference-counted-pointer logic here,
> > please fix the issue there.
> 
> That is fair, however, it still does not fix the core problem.
> 
> Even if we did not need to save the pointer to the parent, we would
> still need to increment the parents reference count, because we have
> to prevent the parent from being released before us.
> 
> There are two ways we can fix the situation:
> 
> 1) We can make it clear that lib/kboject.c does not take any
>    responsibility over the reference counting of the parents, and
>    remove all the parent reference handling from it.
> 
> 2) We fix this by guaranteeing that the parent kboject can not be
>    released before its children.

I think 2 is the key here, and we used to do this, but something got
tweaked over the years with the kobject_cleanup() changes and the fact
that the driver core handles this already for us by incrementing the
parent and doing it in the correct order.

thanks,

greg k-h

Re: [PATCH v1] kobject: make sure parent is not released before children

[Heikki Krogerus]

On Tue, Apr 14, 2020 at 01:42:40PM -0700, Brendan Higgins wrote:
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

This patch isn't from me. You can use a tag like "Proposed-by:
Heikki..." or "Co-developed-by: Heikki..." in cases like this.

> Previously, kobjects were released before the associated kobj_types;
> this can cause a kobject deallocation to fail when the kobject has
> children; an example of this is in software_node_unregister_nodes(); it
> calls release on the parent before children meaning that children can be
> released after the parent, which may be needed for removal.

That makes it sound like it's software_node_unregister_nodes() that is
releasing the parent, which isn't the case.

> So, take a reference to the parent before we delete a node to ensure
> that the parent is not released before the children.
> 
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Fixes: 7589238a8cf3 ("Revert "software node: Simplify software_node_release() function"")
> Link: https://lore.kernel.org/linux-kselftest/CAFd5g44s5NQvT8TG_x4rwbqoa7zWzkV0TX+ETZoQdOB7OwXCPQ@mail.gmail.com/T/#m71f37f3985f2abd7209c8ca8e0fa4edc45e171d6
> Co-developed-by: Brendan Higgins <brendanhiggins@google.com>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---
> 
> This patch is based on the diff written by Heikki linked above.
> 
> Heikki, can you either reply with a Signed-off-by?

No, I can't sign a patch that didn't actually pass trough my hands:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#developer-s-certificate-of-origin-1-1

So in this case this patch is coming from _you_, not me, and there
can't also be a "Signed-off-by: Heikki..." tag.

You can use another tag to tell everybody that the change was
originally proposed by me (I guess it's "Co-developed-by:
Heikki..."?). Or just explain it in the commit message.


thanks,

-- 
heikki