linux-kernel - Re: [PATCH 1/2] i3c: master: Fix confusing cleanup.h syntax

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aTbz2gkfQm9jcHbQ@lizhi-Precision-Tower-5810>
Date: Mon, 8 Dec 2025 10:50:50 -0500
From: Frank Li <Frank.li@....com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@....qualcomm.com>
Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>,
	Jorge Marques <jorge.marques@...log.com>,
	linux-i3c@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] i3c: master: Fix confusing cleanup.h syntax

On Mon, Dec 08, 2025 at 03:07:51AM +0100, Krzysztof Kozlowski wrote:
> Initializing automatic __free variables to NULL without need (e.g.
> branches with different allocations), followed by actual allocation is
> in contrary to explicit coding rules guiding cleanup.h:
>
> "Given that the "__free(...) = NULL" pattern for variables defined at
> the top of the function poses this potential interdependency problem the
> recommendation is to always define and assign variables in one statement
> and not group variable definitions at the top of the function when
> __free() is used."
>
> Code does not have a bug, but is less readable and uses discouraged
> coding practice, so fix that by moving declaration to the place of
> assignment.
>
> Not that other existing usage of __free() in this context is a corret
> exception initialized to NULL, because the actual allocation is branched
> in if().
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@....qualcomm.com>
> ---
>  drivers/i3c/master.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 823661a81f5e..62437a899cdc 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1742,11 +1742,10 @@ EXPORT_SYMBOL_GPL(i3c_master_do_daa);
>  struct i3c_dma *i3c_master_dma_map_single(struct device *dev, void *buf,
>  	size_t len, bool force_bounce, enum dma_data_direction dir)
>  {
> -	struct i3c_dma *dma_xfer __free(kfree) = NULL;
>  	void *bounce __free(kfree) = NULL;
>  	void *dma_buf = buf;
>
> -	dma_xfer = kzalloc(sizeof(*dma_xfer), GFP_KERNEL);
> +	struct i3c_dma *dma_xfer __free(kfree) = kzalloc(sizeof(*dma_xfer), GFP_KERNEL);
>  	if (!dma_xfer)
>  		return NULL;

According to thread
https://lore.kernel.org/all/CAHk-=whPZoi03ZwphxiW6cuWPtC3nyKYS8_BThgztCdgPWP1WA@mail.gmail.com/
"But again: I don't want to make this some kind of hard rule, and I
think it should be done judiciously and with taste, not some kind of
crazy conversion thing."

And there are not logic code between declear and kzalloc.
https://lore.kernel.org/all/CAHk-=wjMMSAaqTjBSfYenfuzE1bMjLj+2DLtLWJuGt07UGCH_Q@mail.gmail.com/
	struct ring_buffer_per_cpu *cpu_buffer __free(kfree);

        cpu_buffer = kzalloc_node(...);

as separate things (but probably next to each other, so that the
"__free(kfree)" part makes sense because the allocation is right
there)."

void *bounce __free(kfree) = NULL, still be there. Only cleanup one is
not enough valuable.

https://lore.kernel.org/all/CAHk-=wjMMSAaqTjBSfYenfuzE1bMjLj+2DLtLWJuGt07UGCH_Q@mail.gmail.com/#t
"   would then make that otherwise nasty allocation then become just

    auto cpu_buffer __free(kfree) = cpu_buffer_alloc();
"

Maybe we can defer this cleanup after kernel using "auto" keywords widely.

Frank




>
> --
> 2.51.0
>