[Linux-kernel-mentees] checkpatch.pl investigation for false-positives

[Linux-kernel-mentees] checkpatch.pl investigation for false-positives

[Ujjwal Kumar]

Hi Lukas,

I looked at different error messages and came-up with a list of
possible false positives reports by checkpatch script.

First, is about BAD_SIGN_OFF warning for Duplicate signature.
Though it is obvious to have these warnings but I am wondering 
about situations where there is some other signature between 
two exactly same signatures.
For instance,

commit 3f9c6d38797e9903937b007a341dad0c251765d6
    ...
    Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
    Acked-by: Andrew F. Davis <afd@ti.com>
    Cc: <Stable@vger.kernel.org>
    Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

commit 79ea1e12c0b8540100e89b32afb9f0e6503fad35
    ...
    Signed-off-by: Johannes Berg <johannes.berg@intel.com>
    Link: https://lore.kernel.org/r/20200604120420.b1dc540a7e26.I55
dcca56bb5bdc5d7ad66a36a0b42afd7034d8be@changeid
    Signed-off-by: Johannes Berg <johannes.berg@intel.com>
    
I'm not familiar how/when these situations can occur. But if these 
scenarios are commonly acceptable then the script should not report
 a warning for duplicate signatures.

---

Second, is about WHILE_AFTER_BRACE errors.

Example commit: 6f8a57ccf8511724e6f48d732cb2940889789ab2

The error message suggests to move the empty while loop with close 
brace of an if block which is not recommended according to the code 
style.

The possible correction would be consider checking the type of 
block that is ending and report only when the do block is found.

---

Third, is about EXECUTE_PERMISSIONS errors where the script reports 
an error for a python (possible with others scripts as well) script 
with file mode set as '100755' but file name does not include '.py' 
extension and is not present under the 'scripts' directory tree.

So, a python script file named 'binfmt_script' with file mode '100755'
 is erroneously reported by the script as:

ERROR:EXECUTE_PERMISSIONS: do not set execute permissions for source files
#39: FILE: tools/testing/selftests/exec/binfmt_script

even when it is okay to make the script file executable.

Example commit b081320f0693cce0394f7c8bad9fba0b25982186

A possible solution to this would be to use the script's first line 
for determining whether the file is a executable file or not.

---

I would like your opinion on these findings, whether any of them can 
be worked on.

Thanks
Ujjwal Kumar
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] checkpatch.pl investigation for false-positives

[Lukas Bulwahn]

On Wed, 23 Sep 2020, Ujjwal Kumar wrote:

> Hi Lukas,
> 
> I looked at different error messages and came-up with a list of
> possible false positives reports by checkpatch script.
> 
> First, is about BAD_SIGN_OFF warning for Duplicate signature.
> Though it is obvious to have these warnings but I am wondering 
> about situations where there is some other signature between 
> two exactly same signatures.
> For instance,
> 
> commit 3f9c6d38797e9903937b007a341dad0c251765d6
>     ...
>     Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>     Acked-by: Andrew F. Davis <afd@ti.com>
>     Cc: <Stable@vger.kernel.org>
>     Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> commit 79ea1e12c0b8540100e89b32afb9f0e6503fad35
>     ...
>     Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>     Link: https://lore.kernel.org/r/20200604120420.b1dc540a7e26.I55
> dcca56bb5bdc5d7ad66a36a0b42afd7034d8be@changeid
>     Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>     
> I'm not familiar how/when these situations can occur. But if these 
> scenarios are commonly acceptable then the script should not report
>  a warning for duplicate signatures.
>

That is a typical situation when a maintainer sends their own patches to a 
mailing list and then picks them up again. Hmm, I need to think about if 
that should be addressed. You could of course detect the Link: 
with lore.kernel.org and identify that all further sign offs are now part 
of the patch integration. Running checkpatch.pl on git commits is not the
most common thing, so it might just be a minor issue.

Let us keep it in mind for now.

> ---
> 
> Second, is about WHILE_AFTER_BRACE errors.
> 
> Example commit: 6f8a57ccf8511724e6f48d732cb2940889789ab2
> 
> The error message suggests to move the empty while loop with close 
> brace of an if block which is not recommended according to the code 
> style.
> 
> The possible correction would be consider checking the type of 
> block that is ending and report only when the do block is found.
>

Okay, that is going to be tricky... maybe clang-format and friends is the 
better tool to handle that eventually. I would not pick that to 
investigate further, just because it is quite tricky. checkpatch.pl is 
already just doing some rough estimation and not really parsing the syntax 
tree.
> ---
> 
> Third, is about EXECUTE_PERMISSIONS errors where the script reports 
> an error for a python (possible with others scripts as well) script 
> with file mode set as '100755' but file name does not include '.py' 
> extension and is not present under the 'scripts' directory tree.
> 
> So, a python script file named 'binfmt_script' with file mode '100755'
>  is erroneously reported by the script as:
> 
> ERROR:EXECUTE_PERMISSIONS: do not set execute permissions for source files
> #39: FILE: tools/testing/selftests/exec/binfmt_script
> 
> even when it is okay to make the script file executable.
> 
> Example commit b081320f0693cce0394f7c8bad9fba0b25982186
> 
> A possible solution to this would be to use the script's first line 
> for determining whether the file is a executable file or not.
>

That seems reasonable. Maybe the rule needs an update anyway because 
executable scripts can also be in tools/...

I suggest to have a look at the discussion of this patch where we already 
discussed out the role of execute permissions in the kernel repository:

https://lore.kernel.org/linux-kernel-mentees/20200827060523.6fi34tcrg5xndzuu@mrinalpandey/#t

Mrinal never followed up on the two potential tasks in
https://lore.kernel.org/linux-kernel-mentees/alpine.DEB.2.21.2008310714560.8556@felia/

Ujjwal, maybe you can look at the build scripts and check if there are any 
places where the build really relies on the file being executable (which 
we currently do guarantee)?

If there are some questions to the task, feel free to ask.

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] checkpatch.pl investigation for false-positives

[Ujjwal Kumar]

On 24/09/20 10:56 pm, Lukas Bulwahn wrote:
> 
> 
> On Wed, 23 Sep 2020, Ujjwal Kumar wrote:
> 
>> Hi Lukas,
>>
>> I looked at different error messages and came-up with a list of
>> possible false positives reports by checkpatch script.
>>
>> First, is about BAD_SIGN_OFF warning for Duplicate signature.
>> Though it is obvious to have these warnings but I am wondering 
>> about situations where there is some other signature between 
>> two exactly same signatures.
>> For instance,
>>
>> commit 3f9c6d38797e9903937b007a341dad0c251765d6
>>     ...
>>     Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>     Acked-by: Andrew F. Davis <afd@ti.com>
>>     Cc: <Stable@vger.kernel.org>
>>     Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>
>> commit 79ea1e12c0b8540100e89b32afb9f0e6503fad35
>>     ...
>>     Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>>     Link: https://lore.kernel.org/r/20200604120420.b1dc540a7e26.I55
>> dcca56bb5bdc5d7ad66a36a0b42afd7034d8be@changeid
>>     Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>>     
>> I'm not familiar how/when these situations can occur. But if these 
>> scenarios are commonly acceptable then the script should not report
>>  a warning for duplicate signatures.
>>
> 
> That is a typical situation when a maintainer sends their own patches to a 
> mailing list and then picks them up again. Hmm, I need to think about if 
> that should be addressed. You could of course detect the Link: 
> with lore.kernel.org and identify that all further sign offs are now part 
> of the patch integration. Running checkpatch.pl on git commits is not the
> most common thing, so it might just be a minor issue.
> 
> Let us keep it in mind for now.
> 
>> ---
>>
>> Second, is about WHILE_AFTER_BRACE errors.
>>
>> Example commit: 6f8a57ccf8511724e6f48d732cb2940889789ab2
>>
>> The error message suggests to move the empty while loop with close 
>> brace of an if block which is not recommended according to the code 
>> style.
>>
>> The possible correction would be consider checking the type of 
>> block that is ending and report only when the do block is found.
>>
> 
> Okay, that is going to be tricky... maybe clang-format and friends is the 
> better tool to handle that eventually. I would not pick that to 
> investigate further, just because it is quite tricky. checkpatch.pl is 
> already just doing some rough estimation and not really parsing the syntax 
> tree.
>> ---
>>
>> Third, is about EXECUTE_PERMISSIONS errors where the script reports 
>> an error for a python (possible with others scripts as well) script 
>> with file mode set as '100755' but file name does not include '.py' 
>> extension and is not present under the 'scripts' directory tree.
>>
>> So, a python script file named 'binfmt_script' with file mode '100755'
>>  is erroneously reported by the script as:
>>
>> ERROR:EXECUTE_PERMISSIONS: do not set execute permissions for source files
>> #39: FILE: tools/testing/selftests/exec/binfmt_script
>>
>> even when it is okay to make the script file executable.
>>
>> Example commit b081320f0693cce0394f7c8bad9fba0b25982186
>>
>> A possible solution to this would be to use the script's first line 
>> for determining whether the file is a executable file or not.
>>
> 
> That seems reasonable. Maybe the rule needs an update anyway because 
> executable scripts can also be in tools/...
> 
> I suggest to have a look at the discussion of this patch where we already 
> discussed out the role of execute permissions in the kernel repository:
> 
> https://lore.kernel.org/linux-kernel-mentees/20200827060523.6fi34tcrg5xndzuu@mrinalpandey/#t
> 
> Mrinal never followed up on the two potential tasks in
> https://lore.kernel.org/linux-kernel-mentees/alpine.DEB.2.21.2008310714560.8556@felia/
> 
> Ujjwal, maybe you can look at the build scripts and check if there are any 
> places where the build really relies on the file being executable (which 
> we currently do guarantee)?
> 

I unset executable bit on all the files, and tried building the kernel.
The configuration step itself fails and hence it appears that build process 
and Makefiles do rely on the executable bits for scripts.

The init/Kconfig file is the reason behind failing builds. I prefixed the 
script calls with relevant interpreters and now I can successfully 
configure and build the kernel. Though I have not tried building all the 
modules (only the ones relevant to my system configuration). I'll be able 
to give a final opinion after making all the modules (my system is too slow
with a dual core cpu :( ).

> If there are some questions to the task, feel free to ask.
> 
> Lukas
> 

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] checkpatch.pl investigation for false-positives

[Lukas Bulwahn]

On Fri, 25 Sep 2020, Ujjwal Kumar wrote:

> On 24/09/20 10:56 pm, Lukas Bulwahn wrote:
> > 
> > 
> > On Wed, 23 Sep 2020, Ujjwal Kumar wrote:
> > 
> >> Hi Lukas,
> >>
> >> I looked at different error messages and came-up with a list of
> >> possible false positives reports by checkpatch script.
> >>
> >> First, is about BAD_SIGN_OFF warning for Duplicate signature.
> >> Though it is obvious to have these warnings but I am wondering 
> >> about situations where there is some other signature between 
> >> two exactly same signatures.
> >> For instance,
> >>
> >> commit 3f9c6d38797e9903937b007a341dad0c251765d6
> >>     ...
> >>     Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >>     Acked-by: Andrew F. Davis <afd@ti.com>
> >>     Cc: <Stable@vger.kernel.org>
> >>     Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >>
> >> commit 79ea1e12c0b8540100e89b32afb9f0e6503fad35
> >>     ...
> >>     Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> >>     Link: https://lore.kernel.org/r/20200604120420.b1dc540a7e26.I55
> >> dcca56bb5bdc5d7ad66a36a0b42afd7034d8be@changeid
> >>     Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> >>     
> >> I'm not familiar how/when these situations can occur. But if these 
> >> scenarios are commonly acceptable then the script should not report
> >>  a warning for duplicate signatures.
> >>
> > 
> > That is a typical situation when a maintainer sends their own patches to a 
> > mailing list and then picks them up again. Hmm, I need to think about if 
> > that should be addressed. You could of course detect the Link: 
> > with lore.kernel.org and identify that all further sign offs are now part 
> > of the patch integration. Running checkpatch.pl on git commits is not the
> > most common thing, so it might just be a minor issue.
> > 
> > Let us keep it in mind for now.
> > 
> >> ---
> >>
> >> Second, is about WHILE_AFTER_BRACE errors.
> >>
> >> Example commit: 6f8a57ccf8511724e6f48d732cb2940889789ab2
> >>
> >> The error message suggests to move the empty while loop with close 
> >> brace of an if block which is not recommended according to the code 
> >> style.
> >>
> >> The possible correction would be consider checking the type of 
> >> block that is ending and report only when the do block is found.
> >>
> > 
> > Okay, that is going to be tricky... maybe clang-format and friends is the 
> > better tool to handle that eventually. I would not pick that to 
> > investigate further, just because it is quite tricky. checkpatch.pl is 
> > already just doing some rough estimation and not really parsing the syntax 
> > tree.
> >> ---
> >>
> >> Third, is about EXECUTE_PERMISSIONS errors where the script reports 
> >> an error for a python (possible with others scripts as well) script 
> >> with file mode set as '100755' but file name does not include '.py' 
> >> extension and is not present under the 'scripts' directory tree.
> >>
> >> So, a python script file named 'binfmt_script' with file mode '100755'
> >>  is erroneously reported by the script as:
> >>
> >> ERROR:EXECUTE_PERMISSIONS: do not set execute permissions for source files
> >> #39: FILE: tools/testing/selftests/exec/binfmt_script
> >>
> >> even when it is okay to make the script file executable.
> >>
> >> Example commit b081320f0693cce0394f7c8bad9fba0b25982186
> >>
> >> A possible solution to this would be to use the script's first line 
> >> for determining whether the file is a executable file or not.
> >>
> > 
> > That seems reasonable. Maybe the rule needs an update anyway because 
> > executable scripts can also be in tools/...
> > 
> > I suggest to have a look at the discussion of this patch where we already 
> > discussed out the role of execute permissions in the kernel repository:
> > 
> > https://lore.kernel.org/linux-kernel-mentees/20200827060523.6fi34tcrg5xndzuu@mrinalpandey/#t
> > 
> > Mrinal never followed up on the two potential tasks in
> > https://lore.kernel.org/linux-kernel-mentees/alpine.DEB.2.21.2008310714560.8556@felia/
> > 
> > Ujjwal, maybe you can look at the build scripts and check if there are any 
> > places where the build really relies on the file being executable (which 
> > we currently do guarantee)?
> > 
> 
> I unset executable bit on all the files, and tried building the kernel.
> The configuration step itself fails and hence it appears that build process 
> and Makefiles do rely on the executable bits for scripts.
> 
> The init/Kconfig file is the reason behind failing builds. I prefixed the 
> script calls with relevant interpreters and now I can successfully 
> configure and build the kernel. Though I have not tried building all the 
> modules (only the ones relevant to my system configuration). I'll be able 
> to give a final opinion after making all the modules (my system is too slow
> with a dual core cpu :( ).
>

If you can share the commands you executed and the patches we need to 
apply, we can apply them and test with further configs and further cores.

Maybe, it is also good just to grep for locations where the Makefile 
introduces new build targets; I think that is only a few places in 
prominent locations. Most of the Makefiles just name files and hook them 
into the normal CC build, adopt compiler warning settings, but do not 
invoke any further special commands.

We are all looking forward to your fixes of the build Makefiles.

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees