linux-kernel - Re: [PATCH] powerpc/ps3: Use struct

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]

Date:   Wed, 16 Jan 2019 09:21:12 -0800
From:   Geoff Levand <geoff@...radead.org>
To:     "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>
Cc:     linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] powerpc/ps3: Use struct_size() in kzalloc()

Hi Gustavo,

On 1/8/19 1:00 PM, Gustavo A. R. Silva wrote:
> One of the more common cases of allocation size calculations is finding the
> size of a structure that has a zero-sized array at the end, along with memory
> for some number of elements for that array. For example:
> 
> struct foo {
>     int stuff;
>     void *entry[];
> };
> 
> instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);
> 
> Instead of leaving these open-coded and prone to type mistakes, we can now
> use the new struct_size() helper:
> 
> instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);
> 
> This code was detected with the help of Coccinelle.
> 
> Signed-off-by: Gustavo A. R. Silva <gustavo@...eddedor.com>
> ---
>  arch/powerpc/platforms/ps3/device-init.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/ps3/device-init.c b/arch/powerpc/platforms/ps3/device-init.c
> index e7075aaff1bb..59587b75493d 100644
> --- a/arch/powerpc/platforms/ps3/device-init.c
> +++ b/arch/powerpc/platforms/ps3/device-init.c
> @@ -354,9 +354,7 @@ static int ps3_setup_storage_dev(const struct ps3_repository_device *repo,
>  		 repo->dev_index, repo->dev_type, port, blk_size, num_blocks,
>  		 num_regions);
>  
> -	p = kzalloc(sizeof(struct ps3_storage_device) +
> -		    num_regions * sizeof(struct ps3_storage_region),
> -		    GFP_KERNEL);
> +	p = kzalloc(struct_size(p, regions, num_regions), GFP_KERNEL);
>  	if (!p) {
>  		result = -ENOMEM;
>  		goto fail_malloc;
It seems to me the motivation for this change is just to have a
code change.  As I've said for other similar patches, I'm reluctant
to accept such trivial changes to the PS3 code because it makes
it harder for me to maintain the code in the long term.  When I
need to do a git bisect to track down a problem I generally have
a set of debugging patches that I apply on top of the bisect.  A
change to the code like this creates the potential for a patch
conflict that I then need to manually edit to resolve.

If this change was for relatively new code, or better, for a patch
that was submitted but not yet merged, then my feelings would be
different.  I think it would be really useful to have something
that scans patches in patchwork.

-Geoff