linux-kernel - Re: [PATCH] checkpatch: add ALLOC_UNNECESSARY

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: <a3a2f6ddbddec0426c76eac655276982e8d58f74.camel@perches.com>
Date:   Sat, 20 Aug 2022 20:47:07 -0400
From:   Joe Perches <joe@...ches.com>
To:     Kenneth Lee <klee33@...edu>, apw@...onical.com,
        dwaipayanray1@...il.com, lukas.bulwahn@...il.com
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH] checkpatch: add ALLOC_UNNECESSARY_ARRAY test

On Sat, 2022-08-20 at 14:41 -0700, Kenneth Lee wrote:
> Using calloc|malloc_array(1, ...) can be simplified to zalloc|malloc(...)
> and improves semantics. This is because we are only allocating one element
> so there is no need to use the array version of the methods.

I don't know if this is particularly useful.

$ git grep -P '(\b(?:devm_(?:kcalloc|kmalloc_array))|\b(?:kv|k)(?:calloc|malloc_array))\s*\(\s*1\s*,' | wc -l
48

If a realloc or equivalent is used to expand the number of elements,
then using 1 as the initial element count seems appropriate.

Have you checked the existing uses?

If these are just using a kcalloc style as a means to zero memory,
then the direct use of the zero allocation equivalent is appropriate.

> This is my first patch that is not a trivial cleanup, so please let me
> know if I am approaching something wrong. Also unsure if the warning
> name should be something else besides ALLOC_UNNECESSARY_ARRAY

Naming is pretty arbitrary, I'm fine with the name.

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -7079,6 +7079,19 @@ sub process {
>  			     "$1 uses number as first arg, sizeof is generally wrong\n" . $herecurr);
>  		}
>  
> +# check for use of unnecessary array alloc methods
> +# calloc|malloc_array(1, ...) should be zalloc|malloc(...)
> +		if ($line =~ /(\b(?:devm_(?:kcalloc|kmalloc_array))|\b(?:kv|k)(?:calloc|malloc_array))\s*\(\s*1\s*,/) {
> +			my $newfunc = "kmalloc";
> +			$newfunc = "kvmalloc" if ($1 eq "kvmalloc_array");
> +			$newfunc = "kvzalloc" if ($1 eq "kvcalloc");
> +			$newfunc = "kzalloc" if ($1 eq "kcalloc");
> +			$newfunc = "devm_kzalloc" if ($1 eq "devm_kcalloc");
> +			$newfunc = "devm_kmalloc" if ($1 eq "devm_kmalloc_array");
> +			WARN("ALLOC_UNNECESSARY_ARRAY",
> +			     "$1(1, ...) can be simplified to $newfunc(...)\n" . $herecurr);

This doesn't work with any alloc using dev_<foo> as these functions
all take a struct device * as the first argument.

And perhaps this would be better/more easily extensible with a hash
instead of a list.

Something like:

our %alloc_array = {
	'devm_kcalloc' => 'devm_kzalloc',
	'devm_kmalloc_array' => 'devm_kmalloc',
	'kvmalloc_array' => 'kvmalloc',
	'kvcalloc' => 'kvzalloc',
	'kcalloc' => 'kzalloc',
	'kmalloc_array' => 'kmalloc',
};

#Create a search pattern for all these strings to speed up a loop below
our $alloc_array_search = "";
foreach my $entry (keys %one_alloc_array) {
	$alloc_array_search .= '|' if ($alloc_array_search ne "");
	$alloc_array_search .= $entry;
}
$alloc_array_search = "(?:${one_alloc_array_search})";
...
		if ($line =~ /(\b($alloc_array)\s*\(\s*1\s*,/) {
			WARN("ALLOC_UNNECESSARY_ARRAY",
			     "$1(1, ...) can be simplified to $alloc_array->$1(...)\n" . $herecurr);

Ideally, this would use $stat and check the positional argument.

Maybe something like:

our @alloc_array = (
	[ 'devm_kcalloc', 'devm_kzalloc', 1 ],
	[ 'devm_kmalloc_array', 'devm_kmalloc', 1 ],
	[ 'kvmalloc_array', 'kvmalloc', 0 ],
	[ 'kvcalloc', 'kvzalloc', 0 ],
	[ 'kcalloc', 'kzalloc', 0 ],
	[ 'kmalloc_array', 'kmalloc', 0 ],
);

#Create a search pattern for all these strings to speed up a loop below
our $alloc_array_search = "";
foreach my $entry (keys %one_alloc_array) {
	$alloc_array_search .= '|' if ($alloc_array_search ne "");
	$alloc_array_search .= $entry->[0];
}
$alloc_array_search = "(?:${one_alloc_array_search})";

Look at the use of mode_permission_funcs for an example of
skipping to a particular argument.