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 <[email protected]>
---
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 <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Thomas Weißschuh <[email protected]>
---
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
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