livepatch: add sysfs entry "patched" for each klp_object

RFC PATCH Superseded Song Liu 3 participants 5 messages Jul 25, 2022 – Aug 3, 2022 Live patching

Share & tools

Song Liu

author maintainer

2022-07-25 22:02 UTC #0 lore raw

RFC PATCH 1/1 livepatch: add sysfs entry "patched" for each klp_object

I was debugging an issue that a livepatch appears to be attached, but actually not. It turns out that there is a mismatch in module name (abc-xyz vs. abc_xyz), klp_find_object_module failed to find the module.

Add per klp_object sysfs entry "patched" to make it easier to debug such issues.

Signed-off-by Song Liu <[email protected]>

---
 kernel/livepatch/core.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

kernel/livepatch/core.c +18 -0

index 5c0d8a4eba13..a9e20c561fef 100644
@@ -364,6 +364,7 @@ static void klp_clear_object_relocations(struct module *pmod,
  * /sys/kernel/livepatch/<patch>/transition
  * /sys/kernel/livepatch/<patch>/force
  * /sys/kernel/livepatch/<patch>/<object>
+ * /sys/kernel/livepatch/<patch>/<object>/patched
  * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
  */
 static int __klp_disable_patch(struct klp_patch *patch);

@@ -470,6 +471,22 @@ static struct attribute *klp_patch_attrs[] = {
 };
 ATTRIBUTE_GROUPS(klp_patch);
 
+static ssize_t patched_show(struct kobject *kobj,
+			    struct kobj_attribute *attr, char *buf)
+{
+	struct klp_object *obj;
+
+	obj = container_of(kobj, struct klp_object, kobj);
+	return snprintf(buf, PAGE_SIZE, "%d\n", obj->patched);
+}
+
+static struct kobj_attribute patched_kobj_attr = __ATTR_RO(patched);
+static struct attribute *klp_object_attrs[] = {
+	&patched_kobj_attr.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(klp_object);
+
 static void klp_free_object_dynamic(struct klp_object *obj)
 {
 	kfree(obj->name);

@@ -615,6 +632,7 @@ static void klp_kobj_release_object(struct kobject *kobj)
 static struct kobj_type klp_ktype_object = {
 	.release = klp_kobj_release_object,
 	.sysfs_ops = &kobj_sysfs_ops,
+	.default_groups = klp_object_groups,
 };
 
 static void klp_kobj_release_func(struct kobject *kobj)

Signature · 1 line

2.30.2

Petr Mladek

maintainer

2022-07-28 13:44 UTC #1 lore raw

Quoted 3 lines · Song Liu

I was debugging an issue that a livepatch appears to be attached, but actually not. It turns out that there is a mismatch in module name (abc-xyz vs. abc_xyz), klp_find_object_module failed to find the module.

This might be a quite common mistake. IMHO, the module name stored in the module metadata always uses underscore '_' instead of dash '-'.

If I get it correctly, it is done by the following command in scripts/Makefile.lib:

--- cut ---
# These flags are needed for modversions and compiling, so we define them here # $(modname_flags) defines KBUILD_MODNAME as the name of the module it will

# end up in (or would, if it gets compiled in)
name-fix-token = $(subst $(comma),_,$(subst -,_,$1))
--- cut ---

It might be worth to check it in klp_init_object() and warn when the module name contains the dash '-'. It would allow to catch this mistake during testing.

Quoted 2 lines

Add per klp_object sysfs entry "patched" to make it easier to debug such issues.

IMHO, this makes sense anyway. It would be useful for debugging any typo in the module name.

Just please, use imperative style for the commit message. For example:

Add per klp_object sysfs entry "patched". It makes it easier to debug typos in the module name.

kernel/livepatch/core.c static void klp_clear_object_relocations(struct module *pmod, · line 370

Quoted 10 lines

kernel/livepatch/core.c +1 -0

@@ -364,6 +364,7 @@ static void klp_clear_object_relocations(struct module *pmod,
  * /sys/kernel/livepatch/<patch>/transition
  * /sys/kernel/livepatch/<patch>/force
  * /sys/kernel/livepatch/<patch>/<object>
+ * /sys/kernel/livepatch/<patch>/<object>/patched
  * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
  */
 static int __klp_disable_patch(struct klp_patch *patch);

Please, document it also in
Documentation/ABI/testing/sysfs-kernel-livepatch

kernel/livepatch/core.c static struct attribute *klp_patch_attrs[] = { · line 480

Quoted 11 lines

kernel/livepatch/core.c +7 -0

@@ -470,6 +471,22 @@ static struct attribute *klp_patch_attrs[] = {
 };
 ATTRIBUTE_GROUPS(klp_patch);
 
+static ssize_t patched_show(struct kobject *kobj,
+			    struct kobj_attribute *attr, char *buf)
+{
+	struct klp_object *obj;
+
+	obj = container_of(kobj, struct klp_object, kobj);
+	return snprintf(buf, PAGE_SIZE, "%d\n", obj->patched);

Please, use:

	return sysfs_emit(buf, "%d\n", obj->patched);

It is a rather new API that should be used to avoid various mistakes.

snprintf() is a common mistake. It returns the length needed to print the entire string. sysfs_emit() uses scnprintf() that returns the really used buffer size and never goes beyond the buffer size.

It would be great to use sysfs_emit() everywhere in the livepatch code. But it needs to be done separately.

kernel/livepatch/core.c static struct attribute *klp_patch_attrs[] = { · line 492

Quoted 12 lines

kernel/livepatch/core.c +9 -0

+}
+
+static struct kobj_attribute patched_kobj_attr = __ATTR_RO(patched);
+static struct attribute *klp_object_attrs[] = {
+	&patched_kobj_attr.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(klp_object);
+
 static void klp_free_object_dynamic(struct klp_object *obj)
 {
 	kfree(obj->name);

Finally, please add a selftest for the new "patched" interface.

Best Regards,
Petr

Song Liu

author maintainer

2022-08-03 05:54 UTC #2 lore raw

Quoted 12 lines · Petr Mladek

Quoted 3 lines · Song Liu
I was debugging an issue that a livepatch appears to be attached, but actually not. It turns out that there is a mismatch in module name (abc-xyz vs. abc_xyz), klp_find_object_module failed to find the module.
This might be a quite common mistake. IMHO, the module name stored in the module metadata always uses underscore '_' instead of dash '-'.
If I get it correctly, it is done by the following command in scripts/Makefile.lib:

Quoted 6 lines

--- cut ---
# These flags are needed for modversions and compiling, so we define them here # $(modname_flags) defines KBUILD_MODNAME as the name of the module it will
# end up in (or would, if it gets compiled in)
name-fix-token = $(subst $(comma),_,$(subst -,_,$1))
--- cut ---

Yeah, I can confirm the "name-fix" makes the change.

Hi Josh and Joe,

I hit this issue while building live patch for OOT module with kpatch-build. Do you have some suggestions on how to fix it? My current workaround is to manually edit the .ko file...

Thanks,
Song

Joe Lawrence

maintainer

2022-08-03 12:36 UTC #3 lore raw

Quoted 20 lines · Song Liu

Quoted 12 lines · Petr Mladek
Quoted 3 lines · Song Liu
I was debugging an issue that a livepatch appears to be attached, but actually not. It turns out that there is a mismatch in module name (abc-xyz vs. abc_xyz), klp_find_object_module failed to find the module.
This might be a quite common mistake. IMHO, the module name stored in the module metadata always uses underscore '_' instead of dash '-'.
If I get it correctly, it is done by the following command in scripts/Makefile.lib:
Quoted 6 lines
--- cut ---
# These flags are needed for modversions and compiling, so we define them here # $(modname_flags) defines KBUILD_MODNAME as the name of the module it will
# end up in (or would, if it gets compiled in)
name-fix-token = $(subst $(comma),_,$(subst -,_,$1))
--- cut ---

Quoted 8 lines

Yeah, I can confirm the "name-fix" makes the change.
Hi Josh and Joe,
I hit this issue while building live patch for OOT module with kpatch-build. Do you have some suggestions on how to fix it? My current workaround is to manually edit the .ko file...

Hi Song,

Was the original issue that the module's filename included '-' dash character(s) while the module name ends up replaced those with '_' underscores?

If that is the case, would you prefer that kpatch-build warn or refuse to create .ko filenames that include '-' characters?

If you have ideas, feel free to post a new issue over on the project's github. No one should have to resort to manually (hex) editing .ko files :)

Signature · 1 line

Joe

Song Liu

author maintainer

2022-08-03 15:46 UTC #4 lore raw

Hi Joe,

Quoted 31 lines · Joe Lawrence

Quoted 20 lines · Song Liu
Quoted 12 lines · Petr Mladek
Quoted 3 lines · Song Liu
I was debugging an issue that a livepatch appears to be attached, but actually not. It turns out that there is a mismatch in module name (abc-xyz vs. abc_xyz), klp_find_object_module failed to find the module.
This might be a quite common mistake. IMHO, the module name stored in the module metadata always uses underscore '_' instead of dash '-'.
If I get it correctly, it is done by the following command in scripts/Makefile.lib:
Quoted 6 lines
--- cut ---
# These flags are needed for modversions and compiling, so we define them here # $(modname_flags) defines KBUILD_MODNAME as the name of the module it will
# end up in (or would, if it gets compiled in)
name-fix-token = $(subst $(comma),_,$(subst -,_,$1))
--- cut ---
Quoted 8 lines
Yeah, I can confirm the "name-fix" makes the change.
Hi Josh and Joe,
I hit this issue while building live patch for OOT module with kpatch-build. Do you have some suggestions on how to fix it? My current workaround is to manually edit the .ko file...

Quoted 5 lines

Hi Song,
Was the original issue that the module's filename included '-' dash character(s) while the module name ends up replaced those with '_' underscores?

There are multiple mismatches: module name, rela section name(s), and rela entry name(s).

Quoted 3 lines

If that is the case, would you prefer that kpatch-build warn or refuse to create .ko filenames that include '-' characters?

I think the best is to have kpatch-build do the same subst for OOT modules, just as when the module itself is built.

Quoted 3 lines

If you have ideas, feel free to post a new issue over on the project's github. No one should have to resort to manually (hex) editing .ko files :)

Yeah, I will create an issue on GitHub.

Thanks,
Song