linux-kernel - Re: [RFC] slab: introduce auto

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: <Z-0SU8cYkTTbprSh@smile.fi.intel.com>
Date: Wed, 2 Apr 2025 13:32:51 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org, vbabka@...e.cz,
	torvalds@...ux-foundation.org, peterz@...radead.org,
	intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org
Subject: Re: [RFC] slab: introduce auto_kfree macro

On Tue, Apr 01, 2025 at 03:44:08PM +0200, Przemek Kitszel wrote:
> Add auto_kfree macro that acts as a higher level wrapper for manual
> __free(kfree) invocation, and sets the pointer to NULL - to have both
> well defined behavior also for the case code would lack other assignement.
> 
> Consider the following code:
> int my_foo(int arg)
> {
> 	struct my_dev_foo *foo __free(kfree); /* no assignement */
> 
> 	foo = kzalloc(sizeof(*foo), GFP_KERNEL);
> 	/* ... */
> }
> 
> So far it is fine and even optimal in terms of not assigning when
> not needed. But it is typical to don't touch (and sadly to don't
> think about) code that is not related to the change, so let's consider
> an extension to the above, namely an "early return" style to check
> arg prior to allocation:
> int my_foo(int arg)
> {
>         struct my_dev_foo *foo __free(kfree); /* no assignement */
> +
> +	if (!arg)
> +		return -EINVAL;
>         foo = kzalloc(sizeof(*foo), GFP_KERNEL);
>         /* ... */
> }
> Now we have uninitialized foo passed to kfree, what likely will crash.
> One could argue that `= NULL` should be added to this patch, but it is
> easy to forgot, especially when the foo declaration is outside of the
> default git context.
> 
> With new auto_kfree, we simply will start with
> 	struct my_dev_foo *foo auto_kfree;
> and be safe against future extensions.
> 
> I believe this will open up way for broader adoption of Scope Based
> Resource Management, say in networking.
> I also believe that my proposed name is special enough that it will
> be easy to know/spot that the assignement is hidden.


I understand the issue and the problem it solves, but...

> +#define auto_kfree __free(kfree) = NULL

...I do not like this syntax at all (note, you forgot to show the result
in the code how it will look like).

What would be better in my opinion is to have it something like DEFINE_*()
type, which will look more naturally in the current kernel codebase
(as we have tons of DEFINE_FOO().

	DEFINE_AUTO_KFREE_VAR(name, struct foo);

with equivalent to

	struct foo *name __free(kfree) = NULL

-- 
With Best Regards,
Andy Shevchenko