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.
> 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?
> Another question is whether the same livepatch could support more
> srcversions of the same module. It might be safe when the livepatched
> functions are compatible. But it would be error prone.
>
> If we wanted to allow support for incompatible modules with the same
> name, we would need to encode the srcversion also into the name
> of the special .klp_rela sections so that we could have separate
> relocations for each variant.
>
I am not sure if supporting more srcversions is necessary. Because
I can't find a senario that more than one version of a module are running
in the system at the same time.
>
> 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.
From my point of view, each build will have a different srcversion generated.
Is it necessary to introduce a "mod->build_id"?
>
>> This function can avoid livepatch from patching the wrong version of the
>> function.
>>
>> The original discussion can be seem on [1]. (Discussion with Joe Lawrence)
>>
>> After the discussion, we think that the actual enforcement of this seems
>> like a job for kernel/livepatch/core.c.
>> Or it should be the process of sys call `init_module` when loading a module.
>>
>> I am here waiting for Request For Comment. Before I do codes.
>
> I am open to accept such a feature. It might improve reliability of
> the livepatching.
>
> Let's see how the code looks like and how complicated would be to
> create such livepatches from sources or using kPatch.
>
> Best Regards,
> Petr
Thanks, Petr. And I will start to code the draft soon. If you have any
suggestions, please be
open to tell me :)
Best Regards
Wardenjohn
