[PATCH 0/4] kobject: properly warn on missing release function

[PATCH 0/4] kobject: properly warn on missing release function

[Thomas Weißschuh]

This series contains:
* Patch 1 & 2: some cleanups for the logging in kobject.c
* Patch 3: Moves the validation of the release function from cleanup()
  to add() so the messages are not shown during shutdown where they are
  hard to see.
* Patch 4: Increases the logging level for the release function
  validation.

Please note that Patch 4 will trigger warnings on boot on at least all
machines with ACPI or block devices.
So this patch should probably not be applied yet.

The block dev part is being worked on here:
https://lore.kernel.org/lkml/20230309-kobj_release-gendisk_integrity-v2-0-761a50d71900@weissschuh.net/

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Thomas Weißschuh (4):
      kobject: define common logging prefix
      kobject: align stacktrace levels to logging message
      kobject: validate ktype release function during add
      kobject: upgrade log of missing release func to warn

 lib/kobject.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)
---
base-commit: 55a21105ecc156495446d8ae75d7d73f66baed7b
change-id: 20230311-kobject-warning-d87a2f7b5e66

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH 1/4] kobject: define common logging prefix

[Thomas Weißschuh]

All log messages start with the prefix "kobject: ".
Deduplicate this by using the pr_fmt() facility.

This makes the very long log strings shorter.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 lib/kobject.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/lib/kobject.c b/lib/kobject.c
index 6e2f0bee3560..09c81ffb8b33 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -10,6 +10,8 @@
  * about using the kobject interface.
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/kobject.h>
 #include <linux/string.h>
 #include <linux/export.h>
@@ -127,7 +129,7 @@ static int fill_kobj_path(const struct kobject *kobj, char *path, int length)
 		*(path + --length) = '/';
 	}
 
-	pr_debug("kobject: '%s' (%p): %s: path = '%s'\n", kobject_name(kobj),
+	pr_debug("'%s' (%p): %s: path = '%s'\n", kobject_name(kobj),
 		 kobj, __func__, path);
 
 	return 0;
@@ -223,7 +225,7 @@ static int kobject_add_internal(struct kobject *kobj)
 		kobj->parent = parent;
 	}
 
-	pr_debug("kobject: '%s' (%p): %s: parent: '%s', set: '%s'\n",
+	pr_debug("'%s' (%p): %s: parent: '%s', set: '%s'\n",
 		 kobject_name(kobj), kobj, __func__,
 		 parent ? kobject_name(parent) : "<NULL>",
 		 kobj->kset ? kobject_name(&kobj->kset->kobj) : "<NULL>");
@@ -359,7 +361,7 @@ static __printf(3, 0) int kobject_add_varg(struct kobject *kobj,
 
 	retval = kobject_set_name_vargs(kobj, fmt, vargs);
 	if (retval) {
-		pr_err("kobject: can not set name properly!\n");
+		pr_err("can not set name properly!\n");
 		return retval;
 	}
 	kobj->parent = parent;
@@ -588,7 +590,7 @@ static void __kobject_del(struct kobject *kobj)
 
 	/* send "remove" if the caller did not do it but sent "add" */
 	if (kobj->state_add_uevent_sent && !kobj->state_remove_uevent_sent) {
-		pr_debug("kobject: '%s' (%p): auto cleanup 'remove' event\n",
+		pr_debug("'%s' (%p): auto cleanup 'remove' event\n",
 			 kobject_name(kobj), kobj);
 		kobject_uevent(kobj, KOBJ_REMOVE);
 	}
@@ -658,16 +660,16 @@ static void kobject_cleanup(struct kobject *kobj)
 	const struct kobj_type *t = get_ktype(kobj);
 	const char *name = kobj->name;
 
-	pr_debug("kobject: '%s' (%p): %s, parent %p\n",
+	pr_debug("'%s' (%p): %s, parent %p\n",
 		 kobject_name(kobj), kobj, __func__, kobj->parent);
 
 	if (t && !t->release)
-		pr_debug("kobject: '%s' (%p): does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n",
+		pr_debug("'%s' (%p): does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n",
 			 kobject_name(kobj), kobj);
 
 	/* 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",
+		pr_debug("'%s' (%p): auto cleanup kobject_del\n",
 			 kobject_name(kobj), kobj);
 		__kobject_del(kobj);
 	} else {
@@ -676,14 +678,14 @@ static void kobject_cleanup(struct kobject *kobj)
 	}
 
 	if (t && t->release) {
-		pr_debug("kobject: '%s' (%p): calling ktype release\n",
+		pr_debug("'%s' (%p): calling ktype release\n",
 			 kobject_name(kobj), kobj);
 		t->release(kobj);
 	}
 
 	/* free name if we allocated it */
 	if (name) {
-		pr_debug("kobject: '%s': free name\n", name);
+		pr_debug("'%s': free name\n", name);
 		kfree_const(name);
 	}
 
@@ -703,8 +705,8 @@ static void kobject_release(struct kref *kref)
 	struct kobject *kobj = container_of(kref, struct kobject, kref);
 #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
 	unsigned long delay = HZ + HZ * get_random_u32_below(4);
-	pr_info("kobject: '%s' (%p): %s, parent %p (delayed %ld)\n",
-		 kobject_name(kobj), kobj, __func__, kobj->parent, delay);
+	pr_info("'%s' (%p): %s, parent %p (delayed %ld)\n",
+		kobject_name(kobj), kobj, __func__, kobj->parent, delay);
 	INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
 
 	schedule_delayed_work(&kobj->release, delay);
@@ -733,7 +735,7 @@ EXPORT_SYMBOL(kobject_put);
 
 static void dynamic_kobj_release(struct kobject *kobj)
 {
-	pr_debug("kobject: (%p): %s\n", kobj, __func__);
+	pr_debug("(%p): %s\n", kobj, __func__);
 	kfree(kobj);
 }
 
@@ -910,7 +912,7 @@ EXPORT_SYMBOL_GPL(kset_find_obj);
 static void kset_release(struct kobject *kobj)
 {
 	struct kset *kset = container_of(kobj, struct kset, kobj);
-	pr_debug("kobject: '%s' (%p): %s\n",
+	pr_debug("'%s' (%p): %s\n",
 		 kobject_name(kobj), kobj, __func__);
 	kfree(kset);
 }

-- 
2.39.2

[PATCH 2/4] kobject: align stacktrace levels to logging message

[Thomas Weißschuh]

Without an explicit level the stacktraces are printed at a default
level.
If this level does not match the one from the logging level it may
happen that the stacktrace is shown without the message or vice versa.

Both these cases are confusing, so make sure the user always sees both,
the message and the stacktrace.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 lib/kobject.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/kobject.c b/lib/kobject.c
index 09c81ffb8b33..f79a434e1231 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -340,7 +340,7 @@ void kobject_init(struct kobject *kobj, const struct kobj_type *ktype)
 		/* do not error out as sometimes we can recover */
 		pr_err("kobject (%p): tried to init an initialized object, something is seriously wrong.\n",
 		       kobj);
-		dump_stack();
+		dump_stack_lvl(KERN_ERR);
 	}
 
 	kobject_init_internal(kobj);
@@ -349,7 +349,7 @@ void kobject_init(struct kobject *kobj, const struct kobj_type *ktype)
 
 error:
 	pr_err("kobject (%p): %s\n", kobj, err_str);
-	dump_stack();
+	dump_stack_lvl(KERN_ERR);
 }
 EXPORT_SYMBOL(kobject_init);
 
@@ -413,7 +413,7 @@ int kobject_add(struct kobject *kobj, struct kobject *parent,
 	if (!kobj->state_initialized) {
 		pr_err("kobject '%s' (%p): tried to add an uninitialized object, something is seriously wrong.\n",
 		       kobject_name(kobj), kobj);
-		dump_stack();
+		dump_stack_lvl(KERN_ERR);
 		return -EINVAL;
 	}
 	va_start(args, fmt);

-- 
2.39.2

[PATCH 3/4] kobject: validate ktype release function during add

[Thomas Weißschuh]

Validating the ktype during cleanup is suboptimal.
Many kobjects are only destroyed during shutdown which makes it hard to
observe the messages.

Instead perform the validation when the object is added.

Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
Link: https://lore.kernel.org/lkml/60b2b66c-22c9-1d38-ed1c-7b7d95e32720@alu.unizg.hr/
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 lib/kobject.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/kobject.c b/lib/kobject.c
index f79a434e1231..68ff8a48b784 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -215,6 +215,10 @@ static int kobject_add_internal(struct kobject *kobj)
 		return -EINVAL;
 	}
 
+	if (kobj->ktype && !kobj->ktype->release)
+		pr_debug("'%s' (%p): does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n",
+			 kobject_name(kobj), kobj);
+
 	parent = kobject_get(kobj->parent);
 
 	/* join kset if set, use it as parent if we do not already have one */
@@ -663,10 +667,6 @@ static void kobject_cleanup(struct kobject *kobj)
 	pr_debug("'%s' (%p): %s, parent %p\n",
 		 kobject_name(kobj), kobj, __func__, kobj->parent);
 
-	if (t && !t->release)
-		pr_debug("'%s' (%p): does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n",
-			 kobject_name(kobj), kobj);
-
 	/* remove from sysfs if the caller did not do it */
 	if (kobj->state_in_sysfs) {
 		pr_debug("'%s' (%p): auto cleanup kobject_del\n",

-- 
2.39.2

Re: [PATCH 3/4] kobject: validate ktype release function during add

[Greg Kroah-Hartman]

On Sat, Mar 11, 2023 at 03:14:48AM +0000, Thomas Weißschuh wrote:
> Validating the ktype during cleanup is suboptimal.
> Many kobjects are only destroyed during shutdown which makes it hard to
> observe the messages.
> 
> Instead perform the validation when the object is added.

As much as I would like to do this, it will cause way too many
false-positives at this point in time, sorry.

Yes, kobjects should always have a release function, but for some, they
are static structures and so do not have them, which is why we only
report the problem when the object is going away as that is when it
matters.

So if you fix up all the in-kernel static kobjects first, then we can
take this type of change, sorry.

Your first 2 are great though, I'll go queue them up next week, thanks
for the cleanups there.

greg k-h

[PATCH 4/4] kobject: upgrade log of missing release func to warn

[Thomas Weißschuh]

The documentation is outspoken in its requirement for a release()
function. Documentation/core-api/kobject.rst:

  One important point cannot be overstated: every kobject must have a
  release() method, and the kobject must persist (in a consistent state)
  until that method is called. If these constraints are not met, the code is
  flawed. Note that the kernel will warn you if you forget to provide a
  release() method.  Do not try to get rid of this warning by providing an
  "empty" release function.

So adapt the logging to actually provide the promised warning.

At the moment there are still kobjects that do not have a release()
function, notably integrity_ktype and acpi_hotplug_profile_ktype.
Therefore leave it at pr_warn().

When the remaining cases are fixed the message could be upgraded to
pr_err() and/or an -EINVAL could be returned.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 lib/kobject.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/kobject.c b/lib/kobject.c
index 68ff8a48b784..8723f477095e 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -215,9 +215,11 @@ static int kobject_add_internal(struct kobject *kobj)
 		return -EINVAL;
 	}
 
-	if (kobj->ktype && !kobj->ktype->release)
-		pr_debug("'%s' (%p): does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n",
-			 kobject_name(kobj), kobj);
+	if (kobj->ktype && !kobj->ktype->release) {
+		pr_warn("'%s' (%p): does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n",
+			kobject_name(kobj), kobj);
+		dump_stack_lvl(KERN_WARNING);
+	}
 
 	parent = kobject_get(kobj->parent);
 

-- 
2.39.2