Re: [PATCH][next] checkpatch: add a couple new alloc functions to alloc with multiplies check

[Kees Cook <keescook@chromium.org>]

On Tue, Sep 12, 2023 at 10:51:22AM -0700, Joe Perches wrote:
> On Tue, 2023-09-12 at 11:04 -0600, Gustavo A. R. Silva wrote:
> > vmalloc() and vzalloc() functions have now 2-factor multiplication
> > argument forms vmalloc_array() and vcalloc(), correspondingly.
> 
> > Add alloc-with-multiplies checks for these new functions.
> > 
> > Link: https://github.com/KSPP/linux/issues/342
> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> > ---
> >  scripts/checkpatch.pl | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 7d16f863edf1..45265d0eee1b 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -7207,17 +7207,19 @@ sub process {
> >  			    "Prefer $3(sizeof(*$1)...) over $3($4...)\n" . $herecurr);
> >  		}
> >  
> > -# check for (kv|k)[mz]alloc with multiplies that could be kmalloc_array/kvmalloc_array/kvcalloc/kcalloc
> > +# check for (kv|k|v)[mz]alloc with multiplies that could be kmalloc_array/kvmalloc_array/vmalloc_array/kvcalloc/kcalloc/vcalloc
> >  		if ($perl_version_ok &&
> >  		    defined $stat &&
> > -		    $stat =~ /^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/) {
> > +		    $stat =~ /^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k|v)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,?/) {
> >  			my $oldfunc = $3;
> >  			my $a1 = $4;
> >  			my $a2 = $10;
> >  			my $newfunc = "kmalloc_array";
> >  			$newfunc = "kvmalloc_array" if ($oldfunc eq "kvmalloc");
> > +			$newfunc = "vmalloc_array" if ($oldfunc eq "vmalloc");
> >  			$newfunc = "kvcalloc" if ($oldfunc eq "kvzalloc");
> >  			$newfunc = "kcalloc" if ($oldfunc eq "kzalloc");
> > +			$newfunc = "vcalloc" if ($oldfunc eq "vzalloc");
> >  			my $r1 = $a1;
> >  			my $r2 = $a2;
> >  			if ($a1 =~ /^sizeof\s*\S/) {
> > @@ -7233,7 +7235,7 @@ sub process {
> >  					 "Prefer $newfunc over $oldfunc with multiply\n" . $herectx) &&
> >  				    $cnt == 1 &&
> >  				    $fix) {
> > -					$fixed[$fixlinenr] =~ s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1 . ' = ' . "$newfunc(" . trim($r1) . ', ' . trim($r2)/e;
> > +					$fixed[$fixlinenr] =~ s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k|v)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1 . ' = ' . "$newfunc(" . trim($r1) . ', ' . trim($r2)/e;
> >  				}
> >  			}
> >  		}
> 
> Seems ok.  Dunno how many more of these there might be like
> devm_ variants, but maybe it'd be better style to use a hash
> with replacement instead of the repeated if block
> 
> Something like:
> ---
>  scripts/checkpatch.pl | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 7d16f863edf1c..bacb63518be14 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -854,6 +854,23 @@ foreach my $entry (keys %deprecated_apis) {
>  }
>  $deprecated_apis_search = "(?:${deprecated_apis_search})";
>  
> +our %alloc_with_multiply_apis = (
> +	"kmalloc"		=> "kmalloc_array",
> +	"kvmalloc"		=> "kvmalloc_array",
> +	"vmalloc"		=> "vmalloc_array",
> +	"kvzalloc"		=> "kvcalloc",
> +	"kzalloc"		=> "kcalloc",
> +	"vzalloc"		=> "vcalloc",
> +);

Yeah, this seems more readable. Gustavo can you send a v2 with this
rework?

Thanks!

-Kees

> +
> +#Create a search pattern for all these strings to speed up a loop below
> +our $alloc_with_multiply_search = "";
> +foreach my $entry (keys %alloc_with_multiply_apis) {
> +	$alloc_with_multiply_search .= '|' if ($alloc_with_multiply_search ne "");
> +	$alloc_with_multiply_search .= $entry;
> +}
> +$alloc_with_multiply_search = "(?:${alloc_with_multiply_search})";
> +
>  our $mode_perms_world_writable = qr{
>  	S_IWUGO		|
>  	S_IWOTH		|
> @@ -7210,14 +7227,11 @@ sub process {
>  # check for (kv|k)[mz]alloc with multiplies that could be kmalloc_array/kvmalloc_array/kvcalloc/kcalloc
>  		if ($perl_version_ok &&
>  		    defined $stat &&
> -		    $stat =~ /^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/) {
> +		    $stat =~ /^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*($alloc_with_multiply_search)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/) {
>  			my $oldfunc = $3;
> +			my $newfunc = $alloc_with_multiply_apis{$oldfunc};
>  			my $a1 = $4;
>  			my $a2 = $10;
> -			my $newfunc = "kmalloc_array";
> -			$newfunc = "kvmalloc_array" if ($oldfunc eq "kvmalloc");
> -			$newfunc = "kvcalloc" if ($oldfunc eq "kvzalloc");
> -			$newfunc = "kcalloc" if ($oldfunc eq "kzalloc");
>  			my $r1 = $a1;
>  			my $r2 = $a2;
>  			if ($a1 =~ /^sizeof\s*\S/) {
> @@ -7233,7 +7247,7 @@ sub process {
>  					 "Prefer $newfunc over $oldfunc with multiply\n" . $herectx) &&
>  				    $cnt == 1 &&
>  				    $fix) {
> -					$fixed[$fixlinenr] =~ s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1 . ' = ' . "$newfunc(" . trim($r1) . ', ' . trim($r2)/e;
> +					$fixed[$fixlinenr] =~ s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*($alloc_with_multiply_search)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1 . ' = ' . "$newfunc(" . trim($r1) . ', ' . trim($r2)/e;
>  				}
>  			}
>  		}
> 

-- 
Kees Cook