linux-kernel - Re: [PATCH v5] checkpatch: extend attributes check to handle more patterns

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: <CABJPP5Cec7jKfaygRaxm-z5gzhg-7cBX1GP46NRQDM6rfhg3Ww@mail.gmail.com>
Date:   Sun, 25 Oct 2020 23:40:54 +0530
From:   Dwaipayan Ray <dwaipayanray1@...il.com>
To:     Joe Perches <joe@...ches.com>
Cc:     linux-kernel-mentees@...ts.linuxfoundation.org,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Lukas Bulwahn <lukas.bulwahn@...il.com>
Subject: Re: [PATCH v5] checkpatch: extend attributes check to handle more patterns

On Sun, Oct 25, 2020 at 11:29 PM Joe Perches <joe@...ches.com> wrote:
>
> On Sun, 2020-10-25 at 15:45 +0530, Dwaipayan Ray wrote:
> > It is generally preferred that the macros from
> > include/linux/compiler_attributes.h are used, unless there
> > is a reason not to.
> >
> > checkpatch currently checks __attribute__ for each of
> > packed, aligned, section, printf, scanf, and weak. Other
> > declarations in compiler_attributes.h are not handled.
> >
> > Add a generic test to check the presence of such attributes.
> > Some attributes require more specific handling and are kept
> > separate.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -6155,50 +6155,95 @@ sub process {
> >                       }
> >               }
> >
> >
> > -# Check for __attribute__ packed, prefer __packed
> > +# Check for compiler attributes
> >               if ($realfile !~ m@\binclude/uapi/@ &&
> > -                 $line =~ /\b__attribute__\s*\(\s*\(.*\bpacked\b/) {
> > -                     WARN("PREFER_PACKED",
> > -                          "__packed is preferred over __attribute__((packed))\n" . $herecurr);
> > -             }
> > +                 $rawline =~ /\b__attribute__\s*\(\s*($balanced_parens)\s*\)/) {
>
> Using $rawline would also change comments and that seems wrong.
> Any reason to use $rawline instead of $line?
>
Hi,
Yes I used $line initially but changed it because quoted
strings were being replaced.

Like:
__attribute__((__section__("_ftrace_events")))

$line in this case was:
__attribute__((__section__("XXXXXXXXXXXXXX")))

While $rawline was:
__attribute__((__section__("_ftrace_events")))

So to avoid this problem I changed to $rawline.

Is there any other alternative available perhaps?
Or should I change back to $rawline?

> []
>
> > +                     if (scalar @conv_array > 0 && $conv_possible == 1) {
> > +                             my $replace = join(' ', @conv_array);
> > +                             if (WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
> > +                                      "$replace is preferred over __attribute__(($attr))\n" . $herecurr) &&
> > +                                     $fix) {
> > +                                     $fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*\Q$attr\E\s*\)\s*\)/$replace/;
>
> I looks it would be useful to add
>                                         $fixed[$fixlinenr] =~ s/\}\Q$replace\E/} $replace/;
> so there's a space added between } and any replacements.
>
Yes I will do that sure.

Thanks,
Dwaipayan.