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
