[Linux-kernel-mentees] [PATCH] checkpatch: add fix option for BAD_SIGN_OFF

Lukas Bulwahn lukas.bulwahn at gmail.com
Tue Dec 8 21:07:24 UTC 2020


On Tue, Dec 8, 2020 at 6:44 PM Aditya <yashsri421 at gmail.com> wrote:
>
> On 8/12/20 10:24 pm, Lukas Bulwahn wrote:
> > On Tue, Dec 8, 2020 at 5:44 PM Aditya <yashsri421 at gmail.com> wrote:
> >>
> >> On 3/12/20 12:06 am, Aditya Srivastava wrote:
> >>> Currently checkpatch warns us if the Co-developed-by line for an author
> >>> isn't immediately followed by the Signed-off-by line.
> >>>
> >>> Generally, this warning occurs because of:
> >>> 1) Absence of Signed-off-by line for the author from the patch
> >>> 2) Misplaced Signed-off-by line for the author in the patch
> >>>
> >>> Provide a simple fix by:
> >>> 1) Inserting Signed-off-by line for the author immediately after the
> >>> Co-developed-by line
> >>>
> >>> 2) Removing Signed-off-by line for the author (if it pre-existed on
> >>> some other line)
> >>>
> >>> Suggested-by: Lukas Bulwahn <lukas.bulwahn at gmail.com>
> >>> Signed-off-by: Aditya Srivastava <yashsri421 at gmail.com>
> >>> ---
> >>>  scripts/checkpatch.pl | 28 ++++++++++++++++++++++++----
> >>>  1 file changed, 24 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> >>> index 5c9f13a97c12..d8f9d4d8b13d 100755
> >>> --- a/scripts/checkpatch.pl
> >>> +++ b/scripts/checkpatch.pl
> >>> @@ -2509,6 +2509,9 @@ sub process {
> >>>       my @setup_docs = ();
> >>>       my $setup_docs = 0;
> >>>
> >>> +     my %signed_off_by_lines;
> >>> +     my %co_developed_by_lines;
> >>> +
> >>>       my $camelcase_file_seeded = 0;
> >>>
> >>>       my $checklicenseline = 1;
> >>> @@ -2794,6 +2797,7 @@ sub process {
> >>>                                       my $ctx = $1;
> >>>                                       my ($email_name, $email_comment, $email_address, $comment1) = parse_email($ctx);
> >>>                                       my ($author_name, $author_comment, $author_address, $comment2) = parse_email($author);
> >>> +                                     $signed_off_by_lines{$ctx} = $fixlinenr;
> >>>
> >>>                                       if ($email_address eq $author_address && $email_name eq $author_name) {
> >>>                                               $author_sob = $ctx;
> >>> @@ -2989,11 +2993,17 @@ sub process {
> >>>                                       }
> >>>                               }
> >>>                               if (!defined $lines[$linenr]) {
> >>> -                                     WARN("BAD_SIGN_OFF",
> >>> -                                             "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline);
> >>> +                                     if (WARN("BAD_SIGN_OFF",
> >>> +                                              "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline) &&
> >>> +                                         $fix) {
> >>> +                                             $co_developed_by_lines{$email} = $fixlinenr;
> >>> +                                     }
> >>>                               } elsif ($rawlines[$linenr] !~ /^\s*signed-off-by:\s*(.*)/i) {
> >>> -                                     WARN("BAD_SIGN_OFF",
> >>> -                                          "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
> >>> +                                     if (WARN("BAD_SIGN_OFF",
> >>> +                                              "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]) &&
> >>> +                                         $fix) {
> >>> +                                             $co_developed_by_lines{$email} = $fixlinenr;
> >>> +                                     }
> >>>                               } elsif ($1 ne $email) {
> >>>                                       WARN("BAD_SIGN_OFF",
> >>>                                            "Co-developed-by and Signed-off-by: name/email do not match \n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
> >>> @@ -7203,6 +7213,16 @@ sub process {
> >>>               ERROR("NOT_UNIFIED_DIFF",
> >>>                     "Does not appear to be a unified-diff format patch\n");
> >>>       }
> >>> +
> >>> +     if (%co_developed_by_lines) {
> >>> +             foreach my $co_developer (keys %co_developed_by_lines) {
> >>> +                     if (exists($signed_off_by_lines{$co_developer})) {
> >>> +                             fix_delete_line($signed_off_by_lines{$co_developer}, "Signed-off-by: $co_developer");
> >>> +                     }
> >>> +                     fix_insert_line($co_developed_by_lines{$co_developer} + 1, "Signed-off-by: $co_developer");
> >>> +             }
> >>> +     }
> >>> +
> >>>       if ($is_patch && $has_commit_log && $chk_signoff) {
> >>>               if ($signoff == 0) {
> >>>                       ERROR("MISSING_SIGN_OFF",
> >>>
> >>
> >
> > Not a big fan of that. Especially because adding and removing
> > Signed-off-by: from a trail has a specific meaning.
> >
> > So that fix can only be applied under very specific conditions. You
> > cannot just sign a document in the name of another person, that is
> > clearly fraud and identity theft.
> >
>
> Hi Lukas
> It is actually mentioned in the kernel docs that if a user is signing
> off as a co-developer, they should also use Signed-off-by line
> followed by their co-developed-by tag.
> Link:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
>
> Please note this line: "Since Co-developed-by: denotes authorship,
> every Co-developed-by: must be immediately followed by a
> Signed-off-by: of the associated co-author."
>
> (Please note the usage of "must" in the above quoted sentence.)
>
> So, what we are doing here is just the same, ie if a user is signing
> off as a Co-developer, he should also include his Signed-off-by line. ie,
>
> 1) If he has missed to add it, we will add it.

Only the person with that identity is allowed to do that. Nobody else.

How do you know who is running this script?

Nobody is allowed to add a Sign-off-by: for another person.
The documentation should clearly state that as well.

> 2) However, if he has added it, but in some other line than after his
> Co-developed-by line, we are placing it in the preferred line, so we
> aren't actually deleting his sign-off.
>

That might be legitimate, but the order of Signed-off-by: has a
meaning itself, so maybe you are breaking that then as well.

>
> > As a patch author, you would need to really carefully think if this
> > fix is valid or not. Do we have examples of patches where this is
> > wrong and where your fix can actually be run by a specific person?
> >
>
> Yes, there are 129 cases of this warning over v4.13..v5.8.
>
> I have tried this fix over few patches where it gave desired result.
> For eg,
>
> E.g. 1) for Commit 951531691c4b ("mm/usercopy: use memory range to be
> accessed for wraparound check"), the signed-off-by line is misplaced,
> and is fixed using this fix option.
> ie.
> Signed-off-by: Prasad Sodagudi <psodagud at codeaurora.org>
> Signed-off-by: Isaac J. Manjarres <isaacm at codeaurora.org>
> Co-developed-by: Prasad Sodagudi <psodagud at codeaurora.org>
> Reviewed-by: William Kucharski <william.kucharski at oracle.com>
>
> becomes:
> Signed-off-by: Isaac J. Manjarres <isaacm at codeaurora.org>
> Co-developed-by: Prasad Sodagudi <psodagud at codeaurora.org>
> Signed-off-by: Prasad Sodagudi <psodagud at codeaurora.org>
> Reviewed-by: William Kucharski <william.kucharski at oracle.com>
>
> Please note that "Signed-off-by: Prasad Sodagudi
> <psodagud at codeaurora.org>" line just gets moved after their
> corresponding Co-developed-by line.
>

Reordering might make sense.

>
> E.g. 2) for Commit 31d851407f90 ("cpuidle: haltpoll: Take 'idle='
> override into account"), the Signed-off-by line is missing, ie does
> not exist for the author, so it is added.
>
> ie,
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan at oracle.com>
> Co-developed-by: Joao Martins <joao.m.martins at oracle.com>
> [ rjw: Subject ]
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki at intel.com>
>
> becomes:
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan at oracle.com>
> Co-developed-by: Joao Martins <joao.m.martins at oracle.com>
> Signed-off-by: Joao Martins <joao.m.martins at oracle.com>
> [ rjw: Subject ]
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki at intel.com>
>
> which I feel is the legitimate correction, as the doc states that
> Co-developed-by tag denotes authorship, so the co-developer should
> also sign-off.
> It is maybe similar to any patch, where user has to take
> responsibility of their changes by adding their Signed-off-by line,
> otherwise the patches aren't accepted.
>

Yes, but how do you check that only Joao Martins runs this script and
nobody else?

That is why this feature is ultimately broken, and I would be very
careful if someone really just said I ran checkpatch --fix for that.

> What do you think?
>
> This patch has some conflicts with the recent next branch. I'll send
> modified rebased patch.
>
> Thanks
> Aditya
>
>
>
> > Lukas
> >
>


More information about the Linux-kernel-mentees mailing list