On Mon 2025-03-10 10:22:19, zhang warden wrote:
>
> Hi, Petr!
>
> > The work with the elf sections is tricky. I would prefer to add
> > .srcversion into struct klp_object, something like:
> >
> > struct klp_object {
> > /* external */
> > const char *name;
> > + const char *srcversion;
> > struct klp_func *funcs;
> > struct klp_callbacks callbacks;
> > [...]
> > }
> >
>
> In fact, I have a though in mind that we can easily use the
> srcversion in `struct module` like:
>
> struct module {
> enum module_state state;
>
> /* Member of list of modules */
> struct list_head list;
>
> /* Unique handle for this module */
> char name[MODULE_NAME_LEN];
>
> #ifdef CONFIG_STACKTRACE_BUILD_ID
> /* Module build ID */
> unsigned char build_id[BUILD_ID_SIZE_MAX];
> #endif
>
> /* Sysfs stuff. */
> struct module_kobject mkobj;
> struct module_attribute *modinfo_attrs;
> const char *version;
> const char *srcversion;
> struct kobject *holders_dir;
>
> /* Exported symbols */
> const struct kernel_symbol *syms;
> const u32 *crcs;
> unsigned int num_syms;
>
> becase when we are loading a livepatch module, the syscall `init_module`
> will be called and we can check the srcversion here.
>
> What I want to do in the elf section is that I want to add a new section
> to store the target module src version inside.
Why do you want to store the target module src version in an elf
section, please?
IMHO, it would be much easier to store it in struct klp_object
as I proposed above. Then the check might be as simple as:
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 51a258c24ff5..dfd7132eec4e 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -104,6 +104,7 @@ struct klp_callbacks {
/**
* struct klp_object - kernel object structure for live patching
* @name: module name (or NULL for vmlinux)
+ * @srcversion compactible srcversion (optional)
* @funcs: function entries for functions to be patched in the object
* @callbacks: functions to be executed pre/post (un)patching
* @kobj: kobject for sysfs resources
@@ -117,6 +118,7 @@ struct klp_callbacks {
struct klp_object {
/* external */
const char *name;
+ const char *srcversion;
struct klp_func *funcs;
struct klp_callbacks callbacks;
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 0cd39954d5a1..61004502e72d 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -66,6 +66,13 @@ static void klp_find_object_module(struct klp_object *obj)
* klp_module_going() instead.
*/
mod = find_module(obj->name);
+
+ /* Do not livepatch an incompatible object. */
+ if (mod && obj->srcversion && mod->srcversion) {
+ if (strcmp(obj->srcversion, mod->srcversion) != 0)
+ goto out;
+ }
+
/*
* Do not mess work of klp_module_coming() and klp_module_going().
* Note that the patch might still be needed before klp_module_going()
@@ -75,7 +82,7 @@ static void klp_find_object_module(struct klp_object *obj)
*/
if (mod && mod->klp_alive)
obj->mod = mod;
-
+out:
rcu_read_unlock_sched();
}
The above code causes that the livepatch would ignore an incompatible
object.
Maybe, you want to return an error instead and block the incompatible
module load.
> > It would be optional. I made just a quick look. It might be enough
> > to check it in klp_find_object_module() and do not set obj->mod
> > when obj->srcversion != NULL and does not match mod->srcversion.
> >
> > Wait! We need to check it also in klp_write_section_relocs().
> > Otherwise, klp_apply_section_relocs() might apply the relocations
> > for non-compatible module.
> >
>
> As previously mentioned, if we can check the srcversion when calling
> the syscall `init_module`, refuse to load the module if the livepatch
> module have srcversion and the srcversion is not equal to the target
> in the system. Can it avoid such relocations problem?
Honestly, I am not sure what you mean by target module src version.
Anyway, you could prevent the module load also when
klp_find_object_module() returns an error.
> > Alternative: I think about using "mod->build_id" instead of "srcversion".
> > It would be even more strict because it would make dependency
> > on a particular build.
> >
> > An advantage is that it is defined also for vmlinux,
> > see vmlinux_build_id. So that we could easily use
> > the same approach also for vmlinux.
> >
> > I do not have strong opinion on this though.
> >
>
> Petr, using "mod->build_id" instead of "srcversion" may not be good.
> Because livepatch can not only handle the function in vmlinux but also
> the function in modules.
I do not understand this urgument.
vmlinux can be identified by build_id stored in "vmlinux_build_id".
And modules can be identified by both build_id and srcversion. Both
information are stored in struct module.
A single livepatch could modify more objects: vmlinux and several
modules. The metadata for each modified object are in "struct
klp_object". The related obect is currently identified only by obj->name.
And we could add more precision identification by setting
also "obj->srcversion" and/or "obj->build_id".
> From my point of view, each build will have a different srcversion generated.
> Is it necessary to introduce a "mod->build_id"?
My understanding is that "srcversion" a kind of checksum of the
sources. I guess that it will be always the same for the sources.
I guess that it is not affected by the compiler or compiler options.
But build_id should be unique with each rebuild. It should be
afftected by the compiler or compiler options.
Note that the compiler options might affect how the functions are
opimized (inlining). And it might affect compactibility of the livepatch.
Best Regards,
Petr
