Re: [PATCH v8 04/13] module: Move livepatch support to a separate file

Petr Mladek
2022-02-25T09:34:17Z

On Tue 2022-02-22 14:12:54, Aaron Tomlin wrote:
> No functional change.
>
> This patch migrates livepatch support (i.e. used during module
> add/or load and remove/or deletion) from core module code into
> kernel/module/livepatch.c. At the moment it contains code to
> persist Elf information about a given livepatch module, only.
>
> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>

> diff --git a/kernel/module/livepatch.c b/kernel/module/livepatch.c
> new file mode 100644
> index 000000000000..486d4ff92719
> --- /dev/null
> +++ b/kernel/module/livepatch.c
> @@ -0,0 +1,74 @@
> + * Persist Elf information about a module. Copy the Elf header,
> + * section header table, section string table, and symtab section
> + * index from info to mod->klp_info.
> + */
> +int copy_module_elf(struct module *mod, struct load_info *info)
> +{
> + unsigned int size, symndx;
> + int ret;
> +
> + size = sizeof(*mod->klp_info);
> + mod->klp_info = kmalloc(size, GFP_KERNEL);
> + if (!mod->klp_info)
> + return -ENOMEM;
> +
> + /* Elf header */
> + size = sizeof(mod->klp_info->hdr);
> + memcpy(&mod->klp_info->hdr, info->hdr, size);
> +
> + /* Elf section header table */
> + size = sizeof(*info->sechdrs) * info->hdr->e_shnum;
> + mod->klp_info->sechdrs = kmemdup(info->sechdrs, size, GFP_KERNEL);
> + if (!mod->klp_info->sechdrs) {
> + ret = -ENOMEM;
> + goto free_info;
> + }
> +
> + /* Elf section name string table */
> + size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
> + mod->klp_info->secstrings = kmemdup(info->secstrings, size, GFP_KERNEL);
> + if (!mod->klp_info->secstrings) {
> + ret = -ENOMEM;
> + goto free_sechdrs;
> + }
> +
> + /* Elf symbol section index */
> + symndx = info->index.sym;
> + mod->klp_info->symndx = symndx;
> +
> + /*
> + * For livepatch modules, core_kallsyms.symtab is a complete
> + * copy of the original symbol table. Adjust sh_addr to point
> + * to core_kallsyms.symtab since the copy of the symtab in module
> + * init memory is freed at the end of do_init_module().
> + */
> + mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long)mod->core_kallsyms.symtab;
> +
> + return 0;

This code include several other well hidden changes:

--- del.p 2022-02-24 16:55:26.570054922 +0100
+++ add.p 2022-02-24 16:56:04.766781394 +0100
@@ -3,14 +3,14 @@
* section header table, section string table, and symtab section
* index from info to mod->klp_info.
*/
-static int copy_module_elf(struct module *mod, struct load_info *info)
+int copy_module_elf(struct module *mod, struct load_info *info)
{
unsigned int size, symndx;
int ret;

size = sizeof(*mod->klp_info);
mod->klp_info = kmalloc(size, GFP_KERNEL);
- if (mod->klp_info == NULL)
+ if (!mod->klp_info)
return -ENOMEM;

/* Elf header */
@@ -20,7 +20,7 @@ static int copy_module_elf(struct module
/* Elf section header table */
size = sizeof(*info->sechdrs) * info->hdr->e_shnum;
mod->klp_info->sechdrs = kmemdup(info->sechdrs, size, GFP_KERNEL);
- if (mod->klp_info->sechdrs == NULL) {
+ if (!mod->klp_info->sechdrs) {
ret = -ENOMEM;
goto free_info;
}
@@ -28,7 +28,7 @@ static int copy_module_elf(struct module
/* Elf section name string table */
size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
mod->klp_info->secstrings = kmemdup(info->secstrings, size, GFP_KERNEL);
- if (mod->klp_info->secstrings == NULL) {
+ if (!mod->klp_info->secstrings) {
ret = -ENOMEM;
goto free_sechdrs;
}
@@ -43,8 +43,7 @@ static int copy_module_elf(struct module
* to core_kallsyms.symtab since the copy of the symtab in module
* init memory is freed at the end of do_init_module().
*/
- mod->klp_info->sechdrs[symndx].sh_addr = \
- (unsigned long) mod->core_kallsyms.symtab;
+ mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long)mod->core_kallsyms.symtab;

return 0;

Please do not do these small coding style changes. It complicates the
review and increases the risk of regressions. Different people
have different preferences. Just imagine that every half a year
someone update style of a code by his personal preferences. The
real changes will then get lost in a lot of noise.

Coding style changes might be acceptable only when the code is
reworked or when it significantly improves readability.

That said. I reviewed and tested this patch and did not find any
problem. Feel free to use:

Reviewed-by: Petr Mladek <pmladek@suse.com>
Tested-by: Petr Mladek <pmladek@suse.com>

Please, take the above as an advice for your future work.

Best Regards,
Petr