linux-kernel - Re: Clarifying confusion of our variable placement rules caused by cleanup.h

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: <bff6d9974e50f7cb27cc2b150ecd6e5e2252ae54.camel@HansenPartnership.com>
Date: Tue, 18 Nov 2025 15:21:10 -0500
From: James Bottomley <James.Bottomley@...senPartnership.com>
To: Steven Rostedt <rostedt@...dmis.org>, Linus Torvalds
	 <torvalds@...ux-foundation.org>
Cc: Bart Van Assche <bvanassche@....org>, ksummit@...ts.linux.dev, Dan
 Williams <dan.j.williams@...el.com>, linux-kernel
 <linux-kernel@...r.kernel.org>, Dan Carpenter <dan.carpenter@...aro.org>
Subject: Re: Clarifying confusion of our variable placement rules caused by
 cleanup.h

On Tue, 2025-11-18 at 14:17 -0500, Steven Rostedt wrote:
> On Tue, 18 Nov 2025 10:38:20 -0800
> Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> 
> >         struct x509_parse_context *ctx __free(kfree) = NULL;
> >         ... other code ...
> >         ctx = kzalloc(sizeof(struct x509_parse_context),
> > GFP_KERNEL);
> > 
> > where you have now split up the whole "this is allocated by
> > kmalloc, and free'd by kfree" into two different sections that are
> > not next to each other.

Yes, I get that; effectively you're declaring an unindented variable
scope.  However, there will be cases where you want an explicit free
(so not a __free variable) and under the rule you propose that should
be allowable as tastefully done.

I've also got to say that the NULL initialization above should be
unnecessary given we can compile time detect if there's potential exit
uninitialized, but I can see that simply declaring where allocated
would avoid having to move the allocation if you did want to introduce
an exit above it in new code.

> I've been doing the above, and was even going to recommend it to
> James. But if it is preferred to declare the __free() variables where
> they are allocated, I'd be much happier.
> 
> I think the code could also be better optimized? I haven't run an
> objcopy to confirm but now early exits do not require calling the
> __free() function on NULL pointers.

Yes, I can confirm that (at least from reading the docs not actually
from disassembling the code).

> 
> Most of my code allocates near the top where I don't find this a
> problem, but I do have a few places of:
> 
> 	struct foo *var __free(kfree) = NULL;
> 
> 	if (ret < 0)
> 		return -ERROR;
> 
> 	[ several more error exits ]
> 
> 	var = kmalloc(..);

Agree: moving the declaration avoids the unnecessary check on exit
(although I'd also presume a good compiler can optimize this away).

There is one last case that hasn't been discussed, which is where you
deliberately introduce an extra scope block for the free and
allocation, so in your example above it would look like

	if (ret < )
		return -ERROR

	[ several more error exits ]

	{
		struct foo *var __free(kfree) = kmalloc(...)

		[...]
	}

I think adding an explicit scope block with no other purpose than to
define the use range of a variable can be beneficial ... with the
obvious caveat that doing it too often causes too much indentation.

It can also help with the entangled lock and free described in the
cleanup.h documentation if the block is paired with a scoped guard.

Regards,

James