linux-kernel - Re: [PATCH 1/1] staging: gpib: gpio: Fix memory allocation style in gpib

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: <eb79a1fc-080d-4be7-9d72-5fa248f48fee@app.fastmail.com>
Date: Tue, 22 Apr 2025 08:19:55 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "Marwin Hormiz" <marwinhormiz@...il.com>,
 "Dave Penkler" <dpenkler@...il.com>,
 "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>
Cc: "Dan Carpenter" <dan.carpenter@...aro.org>, matchstick@...erthere.org,
 "Nihar Chaithanya" <niharchaithanya@...il.com>,
 linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] staging: gpib: gpio: Fix memory allocation style in
 gpib_bitbang.c

On Mon, Apr 21, 2025, at 20:41, Marwin Hormiz wrote:
> Change kzalloc() to use sizeof(*variable) instead of sizeof(struct type)
> to improve code maintainability. This follows the kernel coding style
> recommendation for memory allocations.
>
> Signed-off-by: Marwin Hormiz <marwinhormiz@...il.com>

The build bot reply already shows that you did not even build test
the patch. :(

> @@ -46,10 +46,10 @@
>  			dev_dbg(board->gpib_dev, frm, ## __VA_ARGS__); } \
>  	while (0)
> 
> -#define LINVAL gpiod_get_value(DAV),		\
> +#define LINVAL (gpiod_get_value(DAV),		\
>  		gpiod_get_value(NRFD),		\
>  		gpiod_get_value(NDAC),		\
> -		gpiod_get_value(SRQ)
> +		gpiod_get_value(SRQ))
>  #define LINFMT "DAV: %d	 NRFD:%d  NDAC: %d SRQ: %d"

This change is completely unrelated to the rest of the patch
and not described, aside from being broken. The macro is clearly
confusing, so if you want to imprve this, I would suggest removing
it and instead open-coding the contents.

> @@ -1063,7 +1063,7 @@ static int bb_line_status(const struct gpib_board *board)
> 
>  static int allocate_private(struct gpib_board *board)
>  {
> -	board->private_data = kzalloc(sizeof(struct bb_priv), GFP_KERNEL);
> +	board->private_data = kzalloc(sizeof(*board->private_data), GFP_KERNEL);
>  	if (!board->private_data)
>  		return -1;
>  	return 0;

When you do this type of change, you have to check that the
types are actually the same. The original code makes sense,
your change is broken because 'private_data' as usual refers to
a opaque pointer.

     Arnd