[RFC] slab: introduce auto_kfree macro

[RFC] slab: introduce auto_kfree macro

[Przemek Kitszel]

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.

Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 include/linux/slab.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 98e07e9e9e58..b943be0ce626 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -471,6 +471,7 @@ void kfree_sensitive(const void *objp);
 size_t __ksize(const void *objp);
 
 DEFINE_FREE(kfree, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T))
+#define auto_kfree __free(kfree) = NULL
 DEFINE_FREE(kfree_sensitive, void *, if (_T) kfree_sensitive(_T))
 
 /**
-- 
2.39.3

Re: [RFC] slab: introduce auto_kfree macro

[Andy Shevchenko]

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

Re: [RFC] slab: introduce auto_kfree macro

[Andy Shevchenko]

On Wed, Apr 02, 2025 at 01:32:52PM +0300, Andy Shevchenko wrote:
> 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);

Maybe slightly better name is

	DEFINE_AUTO_KFREE_PTR()

as we expect this to be a pointer.

> with equivalent to
> 
> 	struct foo *name __free(kfree) = NULL

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC] slab: introduce auto_kfree macro

[Vlastimil Babka]

Cc Kees and others from his related efforts:

https://lore.kernel.org/all/20250321202620.work.175-kees@kernel.org/

On 4/1/25 15:44, 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.
> 
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
>  include/linux/slab.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 98e07e9e9e58..b943be0ce626 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -471,6 +471,7 @@ void kfree_sensitive(const void *objp);
>  size_t __ksize(const void *objp);
>  
>  DEFINE_FREE(kfree, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T))
> +#define auto_kfree __free(kfree) = NULL
>  DEFINE_FREE(kfree_sensitive, void *, if (_T) kfree_sensitive(_T))
>  
>  /**

Re: [RFC] slab: introduce auto_kfree macro

[Peter Zijlstra]

On Wed, Apr 02, 2025 at 01:32:51PM +0300, Andy Shevchenko wrote:
> 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.

The compiler *should* complain. But neither GCC nor clang actually
appear to warn in this case.

I don't think we should be making dodgy macros like you propose to work
around this compiler deficiency. Instead I would argue we ought to get
both compilers fixed asap, and then none of this will be needed.

Re: [RFC] slab: introduce auto_kfree macro

[Peter Zijlstra]

On Wed, Apr 02, 2025 at 01:32:51PM +0300, Andy Shevchenko wrote:
> 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);

Still weird. Much better to have the compiler complain about the
obvious use of uninitialized.

Re: [RFC] slab: introduce auto_kfree macro

[Peter Zijlstra]

On Wed, Apr 02, 2025 at 02:19:35PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 02, 2025 at 01:32:51PM +0300, Andy Shevchenko wrote:
> > 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.
> 
> The compiler *should* complain. But neither GCC nor clang actually
> appear to warn in this case.
> 
> I don't think we should be making dodgy macros like you propose to work
> around this compiler deficiency. Instead I would argue we ought to get
> both compilers fixed asap, and then none of this will be needed.

Ah, I think the problem is that the cleanup function takes a pointer to
the object, and pointers to uninitialized values are generally
considered okay.

The compilers would have to explicitly disallow this for the cleanup
functions.

Re: [RFC] slab: introduce auto_kfree macro

[Andy Shevchenko]

On Wed, Apr 02, 2025 at 02:21:04PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 02, 2025 at 01:32:51PM +0300, Andy Shevchenko wrote:
> > 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);
> 
> Still weird. Much better to have the compiler complain about the
> obvious use of uninitialized.

That would be ideal!

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC] slab: introduce auto_kfree macro

[Andy Shevchenko]

On Wed, Apr 02, 2025 at 02:22:24PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 02, 2025 at 02:19:35PM +0200, Peter Zijlstra wrote:
> > On Wed, Apr 02, 2025 at 01:32:51PM +0300, Andy Shevchenko wrote:
> > > 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.
> > 
> > The compiler *should* complain. But neither GCC nor clang actually
> > appear to warn in this case.
> > 
> > I don't think we should be making dodgy macros like you propose to work
> > around this compiler deficiency. Instead I would argue we ought to get
> > both compilers fixed asap, and then none of this will be needed.
> 
> Ah, I think the problem is that the cleanup function takes a pointer to
> the object, and pointers to uninitialized values are generally
> considered okay.
> 
> The compilers would have to explicitly disallow this for the cleanup
> functions.

Hmm... What I have heard is that the cleanup is basically a port of
C++ destructor code to C, and it might be related to the virtual functions
that are may be absent for the basic classes. But not an expert here,
just speculating based on my poor knowledge of C++.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC] slab: introduce auto_kfree macro

[Kees Cook]

On Wed, Apr 02, 2025 at 12:44:50PM +0200, Vlastimil Babka wrote:
> Cc Kees and others from his related efforts:
> 
> https://lore.kernel.org/all/20250321202620.work.175-kees@kernel.org/

I think, unfortunately, the consensus is that "invisible side-effects"
are not going to be tolerated. After I finish with kmalloc_obj(), I'd
like to take another run at this for basically providing something like:

static inline __must_check
void *kfree(void *p) { __kfree(p); return NULL; }

And then switch all:

	kfree(s->ptr);

to

	s->ptr = kfree(s->ptr);

Where s->ptr isn't used again.

-Kees

> 
> On 4/1/25 15:44, 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.
> > 
> > Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> > ---
> >  include/linux/slab.h | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/include/linux/slab.h b/include/linux/slab.h
> > index 98e07e9e9e58..b943be0ce626 100644
> > --- a/include/linux/slab.h
> > +++ b/include/linux/slab.h
> > @@ -471,6 +471,7 @@ void kfree_sensitive(const void *objp);
> >  size_t __ksize(const void *objp);
> >  
> >  DEFINE_FREE(kfree, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T))
> > +#define auto_kfree __free(kfree) = NULL
> >  DEFINE_FREE(kfree_sensitive, void *, if (_T) kfree_sensitive(_T))
> >  
> >  /**
> 

-- 
Kees Cook

Re: [RFC] slab: introduce auto_kfree macro

[Matthew Wilcox]

On Thu, Apr 03, 2025 at 09:59:41AM -0700, Kees Cook wrote:
> On Wed, Apr 02, 2025 at 12:44:50PM +0200, Vlastimil Babka wrote:
> > Cc Kees and others from his related efforts:
> > 
> > https://lore.kernel.org/all/20250321202620.work.175-kees@kernel.org/
> 
> I think, unfortunately, the consensus is that "invisible side-effects"
> are not going to be tolerated. After I finish with kmalloc_obj(), I'd
> like to take another run at this for basically providing something like:
> 
> static inline __must_check
> void *kfree(void *p) { __kfree(p); return NULL; }
> 
> And then switch all:
> 
> 	kfree(s->ptr);
> 
> to
> 
> 	s->ptr = kfree(s->ptr);
> 
> Where s->ptr isn't used again.

Umm ... kfree is now going to be __must_check?  That's a lot of churn.

I'd just go with making kfree() return NULL and leave off the
__must_check.  It doesn't need the __kfree() indirection either.
That lets individual functions opt into the new safety.

Re: [RFC] slab: introduce auto_kfree macro

[Andy Shevchenko]

On Thu, Apr 03, 2025 at 06:35:39PM +0100, Matthew Wilcox wrote:
> On Thu, Apr 03, 2025 at 09:59:41AM -0700, Kees Cook wrote:
> > On Wed, Apr 02, 2025 at 12:44:50PM +0200, Vlastimil Babka wrote:
> > > Cc Kees and others from his related efforts:
> > > 
> > > https://lore.kernel.org/all/20250321202620.work.175-kees@kernel.org/
> > 
> > I think, unfortunately, the consensus is that "invisible side-effects"
> > are not going to be tolerated. After I finish with kmalloc_obj(), I'd
> > like to take another run at this for basically providing something like:
> > 
> > static inline __must_check
> > void *kfree(void *p) { __kfree(p); return NULL; }
> > 
> > And then switch all:
> > 
> > 	kfree(s->ptr);
> > 
> > to
> > 
> > 	s->ptr = kfree(s->ptr);
> > 
> > Where s->ptr isn't used again.
> 
> Umm ... kfree is now going to be __must_check?  That's a lot of churn.
> 
> I'd just go with making kfree() return NULL and leave off the
> __must_check.  It doesn't need the __kfree() indirection either.
> That lets individual functions opt into the new safety.

Maybe something like

void kfree_and_null(void **ptr)
{
	__kfree(*ptr);
	*ptr = NULL;
}

?

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC] slab: introduce auto_kfree macro

[Linus Torvalds]

On Tue, 1 Apr 2025 at 06:50, Przemek Kitszel
<przemyslaw.kitszel@intel.com> wrote:
>
> With new auto_kfree, we simply will start with
>         struct my_dev_foo *foo auto_kfree;
> and be safe against future extensions.

Honestly, I'd much rather just see any auto-freeing *always* assigning
the value.

We used to have a rule that declarations had to be at the top of a
block, which made that not an option, but that rule went out the
window exactly because you can't do that with the cleanup macros.

So just make the rule be that __free() without an assignment is simply a bug.

               Linus

Re: [RFC] slab: introduce auto_kfree macro

[Herbert Xu]

Peter Zijlstra <peterz@infradead.org> wrote:
>
> The compiler *should* complain. But neither GCC nor clang actually
> appear to warn in this case.

Linus turned that warning off in 2020:

commit 78a5255ffb6a1af189a83e493d916ba1c54d8c75
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Sat May 9 13:57:10 2020 -0700

    Stop the ad-hoc games with -Wno-maybe-initialized

You need to enable it by hand to see the warning:

make KBUILD_CFLAGS_KERNEL=-Wmaybe-uninitialized CFLAGS_MODULE=-Wmaybe-uninitialized

W=2 enables it too but it also enables lots of other crap so it's
useless.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt