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@xxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> Fixes: 7589238a8cf3 ("Revert "software node: Simplify software_node_release() function"")
> Link: https://lore.kernel.org/linux-kselftest/CAFd5g44s5NQvT8TG_x4rwbqoa7zWzkV0TX+ETZoQdOB7OwXCPQ@xxxxxxxxxxxxxx/T/#m71f37f3985f2abd7209c8ca8e0fa4edc45e171d6
> Co-developed-by: Brendan Higgins <brendanhiggins@xxxxxxxxxx>
> Signed-off-by: Brendan Higgins <brendanhiggins@xxxxxxxxxx>
> ---
> 
> 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