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

[Petr Mladek <pmladek@suse.com>]

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