linux-kernel - Re: [PATCH 1/2] checkpatch: special case extern struct in .c

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] [day] [month] [year] [list]

Message-ID: <95b681074a36807e25351f9de8cb2b23aa5c7ef8.camel@perches.com>
Date:   Thu, 07 Sep 2023 11:18:42 -0700
From:   Joe Perches <joe@...ches.com>
To:     Jim Cromie <jim.cromie@...il.com>, linux-kernel@...r.kernel.org
Cc:     akpm@...ux-foundation.org, apw@...onical.com,
        Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH 1/2] checkpatch: special case extern struct in .c

On Thu, 2023-09-07 at 11:44 -0600, Jim Cromie wrote:
> The warning "externs should be avoided in .c files" wants an exception
> for linker symbols (named in vmlinux.lds.h etc), like those that mark
> the __start, __stop/__end symbols delimiting many kernel sections.
> 
> Since checkpatch already checks REALNAME to avoid looking at patch
> chunks changing vmlinux.lds.h, add a new else-if block to look at them
> instead.  As a simple heuristic, treat all words (in the + patch-lines)
> as candidate symbols, to screen later warnings about the same symbols
> being found in following chunks that change *.c files.
> 
> Where the "# check for new externs in .c files." is done, precede it
> with a new else-if block to isolate one common extern-in-c use case:
> "extern struct foo bar[]".  For this case, we can issue a more
> informative warning:
> 
>   WARN("AVOID_EXTERNS",
>      "found a file-scoped extern type:$st_type name:$st_name in .c file\n"
>      . "is this a linker symbol ?\n" . $herecurr);
> 
> NOTE: The "screening" is a regex match, not an exact match.  This
> accepts __start_foo and __stop_foo symbols found in a *.c file, if
> "foo" was found previously in a vmlinux.lds.h chunk.
> 
> It does require that the patch adding "externs in .c's" also have the
> additions to vmlinux.lds.h.  And it requires vmlinux.lds.h chunks
> before .c chunks.
> 
> Cc: apw@...onical.com
> Cc: joe@...ches.com
> Cc: Kees Cook <keescook@...omium.org>
> Signed-off-by: Jim Cromie <jim.cromie@...il.com>
> ---
>  scripts/checkpatch.pl | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 880fde13d9b8..6aabcc1f66c1 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -74,6 +74,8 @@ my $git_command ='export LANGUAGE=en_US.UTF-8; git';
>  my $tabsize = 8;
>  my ${CONFIG_} = "CONFIG_";
>  
> +my %maybe_linker_symbol; # for externs in c exceptions, when seen in *vmlinux.lds.h
> +
>  sub help {
>  	my ($exitcode) = @_;
>  
> @@ -6051,6 +6053,9 @@ sub process {
>  
>  # check for line continuations outside of #defines, preprocessor #, and asm
>  
> +		} elsif ($realfile =~ m@...linux.lds.h$@) {
> +		    $line =~ s/(\w+)/$maybe_linker_symbol{$1}++/ge;
> +		    #print "REAL: $realfile\nln: $line\nkeys:", sort keys %maybe_linker_symbol;
>  		} else {
>  			if ($prevline !~ /^..*\\$/ &&
>  			    $line !~ /^\+\s*\#.*\\$/ &&		# preprocessor
> @@ -7119,6 +7124,21 @@ sub process {
>  				     "arguments for function declarations should follow identifier\n" . $herecurr);
>  			}
>  
> +		} elsif ($realfile =~ /\.c$/ && defined $stat &&
> +		    $stat =~ /^\+extern struct\s+(\w+)\s+(\w+)\[\];/)

Use the proper \s+ instead of ' '
And why use $stat instead of $sline?
Are you expecting these externs to be on multiple lines?

		} elsif ($realfile =~ /\.c$/ &&
			 $sline =~ /^\+\s*extern\s+struct\s+(\w+)\s+(\w+)\s*\[\s*\]\s*;/


> +		{
> +			my ($st_type, $st_name) = ($1, $2);
> +
> +			for my $s (keys %maybe_linker_symbol) {
> +			    #print "Linker symbol? $st_name : $s\n";
> +			    goto LIKELY_LINKER_SYMBOL

yuck.  no gotos please

Just use last

> +				if $st_name =~ /$s/;
> +			}
> +			WARN("AVOID_EXTERNS",
> +			     "found a file-scoped extern type:$st_type name:$st_name in .c file\n"
> +			     . "is this a linker symbol ?\n" . $herecurr);

Single line output required then $herecurr
Using "in .c file" is also unnecessary.

> +		  LIKELY_LINKER_SYMBOL:
> +
>  		} elsif ($realfile =~ /\.c$/ && defined $stat &&
>  		    $stat =~ /^.\s*extern\s+/)
>  		{