Re: [Intel-wired-lan] [RFC] slab: introduce auto_kfree macro

[Kees Cook <kees@kernel.org>]

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