| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 25 Feb 2022 16:49:31 +0000
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Petr Mladek <pmladek@...e.com>, Aaron Tomlin <atomlin@...hat.com>
CC: "mcgrof@...nel.org" <mcgrof@...nel.org>,
"cl@...ux.com" <cl@...ux.com>, "mbenes@...e.cz" <mbenes@...e.cz>,
"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
"jeyu@...nel.org" <jeyu@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-modules@...r.kernel.org" <linux-modules@...r.kernel.org>,
"void@...ifault.com" <void@...ifault.com>,
"atomlin@...mlin.com" <atomlin@...mlin.com>,
"allen.lkml@...il.com" <allen.lkml@...il.com>,
"joe@...ches.com" <joe@...ches.com>,
"msuchanek@...e.de" <msuchanek@...e.de>,
"oleksandr@...alenko.name" <oleksandr@...alenko.name>
Subject: Re: [PATCH v8 04/13] module: Move livepatch support to a separate
file
Le 25/02/2022 à 10:34, Petr Mladek a écrit :
> 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@...hat.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)
That's not a hidden change. That's part of the move, that's required.
> {
> 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.
I disagree here. We are not talking about people's preference here but
compliance with documented Linux kernel Codying Style and handling of
official checkpatch.pl script reports.
You are right that randomly updating the style every half a year would
be a nightmare and would kill blamability of changes.
However when moving big peaces of code like this, blamability is broken
anyway and this is a very good opportunity to increase compliance of
kernel code to its own codying style. But doing it in several steps
increases code churn and has no real added value.
>
> Coding style changes might be acceptable only when the code is
> reworked or when it significantly improves readability.
When code is moved around it is also a good opportunity.
>
>
> That said. I reviewed and tested this patch and did not find any
> problem. Feel free to use:
>
> Reviewed-by: Petr Mladek <pmladek@...e.com>
> Tested-by: Petr Mladek <pmladek@...e.com>
>
> Please, take the above as an advice for your future work.
>
Thanks
Christophe
Powered by blists - more mailing lists