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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ed8bb968-0a88-39cf-f388-032e8c205df7@csgroup.eu>
Date:   Mon, 28 Feb 2022 11:46:33 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Petr Mladek <pmladek@...e.com>
CC:     Aaron Tomlin <atomlin@...hat.com>,
        "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 28/02/2022 à 11:56, Petr Mladek a écrit :
> On Fri 2022-02-25 16:49:31, Christophe Leroy wrote:
>>
>>
>> Le 25/02/2022 à 10:34, Petr Mladek a écrit :
>>>
>>> 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.
> 
> Really?
> 
> 1. I restored
> 
> 	+	if (mod->klp_info->secstrings == NULL) {
> 
>     and checkpatch.pl is happy.

On mainline's kernel/module.c checkpatch.pl tells me:

CHECK: Comparison to NULL could be written "!mod->klp_info->secstrings"
#2092: FILE: kernel/module.c:2092:
+	if (mod->klp_info->secstrings == NULL) {



> 
> 
> 2. I do not see anythinkg about if (xxx == NULL) checks in
>     Documentation/process/coding-style.rst
> 
> 3. $> git grep "if (.* == NULL" | wc -l
>     15041

Commit b75ac618df75 ("checkpatch: add --strict "pointer comparison to 
NULL" test")

> 
> 4. The result of
> 	-	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;
> 
>     is 90 characeters long and Documentation/process/coding-style.rst says:

Probably a misinterpretation of:

WARNING: Avoid unnecessary line continuations
#2107: FILE: kernel/module.c:2107:
+	mod->klp_info->sechdrs[symndx].sh_addr = \

> 
> 	2) Breaking long lines and strings
> 	----------------------------------
> 
> 	Coding style is all about readability and maintainability using commonly
> 	available tools.
> 
> 	The preferred limit on the length of a single line is 80 columns.
> 
> 	Statements longer than 80 columns should be broken into sensible chunks,
> 	unless exceeding 80 columns significantly increases readability and does
> 	not hide information.
> 
>     checkpatch.pl accepts lines up to 100 columns but 80 are still
>     preferred.
> 
> 
>> 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.
> 
>  From Documentation/process/submitting-patches.rst:
> 
> 	One significant exception is when moving code from one file to
> 	another -- in this case you should not modify the moved code at all in
> 	the same patch which moves it.  This clearly delineates the act of
> 	moving the code and your changes.  This greatly aids review of the
> 	actual differences and allows tools to better track the history of
> 	the code itself.
> 
> 
>>>
>>> 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.
> 
> No!


By the way some maintainers require checkpatch' clean patches even when 
this is only code move. I remember being requested to do that in the 
past, so now I almost always do it with my own patches.

> 
> I would not have complained if it did not complicate my review.
> But it did!
> 

Reviewing partial code move is not easy anyway, git is not very 
userfriendly with that.

Christophe