[PATCH RFC] livepatch: add sysfs entry "patched" for each klp_object

[PATCH RFC] livepatch: add sysfs entry "patched" for each klp_object

[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.

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

Signed-off-by: Song Liu <song@kernel.org>
---
 kernel/livepatch/core.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 5c0d8a4eba13..a9e20c561fef 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -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)
-- 
2.30.2

Re: [PATCH RFC] livepatch: add sysfs entry "patched" for each klp_object

[Petr Mladek]

On Mon 2022-07-25 15:02:31, Song Liu wrote:
> 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.


> 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.


> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -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

> @@ -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.

> +}
> +
> +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

Re: [PATCH RFC] livepatch: add sysfs entry "patched" for each klp_object

[Song Liu]

On Thu, Jul 28, 2022 at 6:44 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Mon 2022-07-25 15:02:31, Song Liu wrote:
> > 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 ---

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

Re: [PATCH RFC] livepatch: add sysfs entry "patched" for each klp_object

[Joe Lawrence]

On 8/3/22 1:53 AM, Song Liu wrote:
> On Thu, Jul 28, 2022 at 6:44 AM Petr Mladek <pmladek@suse.com> wrote:
>>
>> On Mon 2022-07-25 15:02:31, Song Liu wrote:
>>> 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 ---
> 
> 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 :)

-- 
Joe

Re: [PATCH RFC] livepatch: add sysfs entry "patched" for each klp_object

[Song Liu]

Hi Joe,

On Wed, Aug 3, 2022 at 5:36 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>
> On 8/3/22 1:53 AM, Song Liu wrote:
> > On Thu, Jul 28, 2022 at 6:44 AM Petr Mladek <pmladek@suse.com> wrote:
> >>
> >> On Mon 2022-07-25 15:02:31, Song Liu wrote:
> >>> 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 ---
> >
> > 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?

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

>
> 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.

>
> 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