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

Lukas Bulwahn lukas.bulwahn at gmail.com
Tue Dec 8 16:54:09 UTC 2020


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.

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?

Lukas


More information about the Linux-kernel-mentees mailing list