[PATCH] mm/slab: make __free(kfree) accept error pointers

[PATCH] mm/slab: make __free(kfree) accept error pointers

[Dan Carpenter]

Currently, if an automatically freed allocation is an error pointer that
will lead to a crash.  An example of this is in wm831x_gpio_dbg_show().

   171	char *label __free(kfree) = gpiochip_dup_line_label(chip, i);
   172	if (IS_ERR(label)) {
   173		dev_err(wm831x->dev, "Failed to duplicate label\n");
   174		continue;
   175  }

The auto clean up function should check for error pointers as well,
otherwise we're going to keep hitting issues like this.

Fixes: 54da6a092431 ("locking: Introduce __cleanup() based infrastructure")
Cc: <stable@vger.kernel.org>
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
Obviously, the fixes tag isn't very fair but it will tell the -stable
tools how far to backport this.

 include/linux/slab.h  | 4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 4cc37ef22aae..5f5766219375 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -279,7 +279,7 @@ void kfree(const void *objp);
 void kfree_sensitive(const void *objp);
 size_t __ksize(const void *objp);
 
-DEFINE_FREE(kfree, void *, if (_T) kfree(_T))
+DEFINE_FREE(kfree, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T))
 
 /**
  * ksize - Report actual allocation size of associated object
@@ -808,7 +808,7 @@ extern void *kvrealloc_noprof(const void *p, size_t oldsize, size_t newsize, gfp
 #define kvrealloc(...)				alloc_hooks(kvrealloc_noprof(__VA_ARGS__))
 
 extern void kvfree(const void *addr);
-DEFINE_FREE(kvfree, void *, if (_T) kvfree(_T))
+DEFINE_FREE(kvfree, void *, if (!IS_ERR_OR_NULL(_T)) kvfree(_T))
 
 extern void kvfree_sensitive(const void *addr, size_t len);
 
-- 
2.43.0

Re: [PATCH] mm/slab: make __free(kfree) accept error pointers

[David Rientjes]

On Sun, 28 Apr 2024, Dan Carpenter wrote:

> Currently, if an automatically freed allocation is an error pointer that
> will lead to a crash.  An example of this is in wm831x_gpio_dbg_show().
> 
>    171	char *label __free(kfree) = gpiochip_dup_line_label(chip, i);
>    172	if (IS_ERR(label)) {
>    173		dev_err(wm831x->dev, "Failed to duplicate label\n");
>    174		continue;
>    175  }
> 
> The auto clean up function should check for error pointers as well,
> otherwise we're going to keep hitting issues like this.
> 
> Fixes: 54da6a092431 ("locking: Introduce __cleanup() based infrastructure")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Acked-by: David Rientjes <rientjes@google.com>

Re: [PATCH] mm/slab: make __free(kfree) accept error pointers

[Matthew Wilcox]

On Sun, Apr 28, 2024 at 05:26:44PM +0300, Dan Carpenter wrote:
> Currently, if an automatically freed allocation is an error pointer that
> will lead to a crash.  An example of this is in wm831x_gpio_dbg_show().
> 
>    171	char *label __free(kfree) = gpiochip_dup_line_label(chip, i);
>    172	if (IS_ERR(label)) {
>    173		dev_err(wm831x->dev, "Failed to duplicate label\n");
>    174		continue;
>    175  }
> 
> The auto clean up function should check for error pointers as well,
> otherwise we're going to keep hitting issues like this.
> 
> Fixes: 54da6a092431 ("locking: Introduce __cleanup() based infrastructure")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> Obviously, the fixes tag isn't very fair but it will tell the -stable
> tools how far to backport this.
> 
>  include/linux/slab.h  | 4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 4cc37ef22aae..5f5766219375 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -279,7 +279,7 @@ void kfree(const void *objp);
>  void kfree_sensitive(const void *objp);
>  size_t __ksize(const void *objp);
>  
> -DEFINE_FREE(kfree, void *, if (_T) kfree(_T))
> +DEFINE_FREE(kfree, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T))

Wait, why do we check 'if (_T)' at all?  kfree() already handles NULL
pointers just fine.  I wouldn't be averse to making it handle error
pointers either.

> -DEFINE_FREE(kvfree, void *, if (_T) kvfree(_T))
> +DEFINE_FREE(kvfree, void *, if (!IS_ERR_OR_NULL(_T)) kvfree(_T))

Ditto kvfree().  Fixing kfree() would fix both of these.

Re: [PATCH] mm/slab: make __free(kfree) accept error pointers

[Dan Carpenter]

On Mon, Apr 29, 2024 at 04:03:07AM +0100, Matthew Wilcox wrote:
> > diff --git a/include/linux/slab.h b/include/linux/slab.h
> > index 4cc37ef22aae..5f5766219375 100644
> > --- a/include/linux/slab.h
> > +++ b/include/linux/slab.h
> > @@ -279,7 +279,7 @@ void kfree(const void *objp);
> >  void kfree_sensitive(const void *objp);
> >  size_t __ksize(const void *objp);
> >  
> > -DEFINE_FREE(kfree, void *, if (_T) kfree(_T))
> > +DEFINE_FREE(kfree, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T))
> 
> Wait, why do we check 'if (_T)' at all?  kfree() already handles NULL
> pointers just fine.  I wouldn't be averse to making it handle error
> pointers either.
> 
> > -DEFINE_FREE(kvfree, void *, if (_T) kvfree(_T))
> > +DEFINE_FREE(kvfree, void *, if (!IS_ERR_OR_NULL(_T)) kvfree(_T))
> 
> Ditto kvfree().  Fixing kfree() would fix both of these.

I've always thought freeing pointers that have not been allocated is
sloppy so I like that kfree() doesn't allow error pointers.  We always
catch it before it reaches production and that teaches people better
habbits.  Personally, I like how free_netdev() only accepts valid
pointers.

But I won't fight you on that if you want to change it.  People have
discussed this in the past, but no one has actually sent the patch.  It
would probably be merged.

The __free() stuff is different because it's supposed to be transparent.

Btw, I'm hoping we can officially declare small allocations as NOFAIL so
then we can start doing allocations in the declaration block and remove
the error checking and the cleanup.

#define __ALLOC(p) p __free(kfree) = kmalloc(sizeof(*p), GFP_SMALL)
#define __ZALLOC(p) p __free(kfree) = kzalloc(sizeof(*p), GFP_SMALL)

	struct foo *__ZALLOC(p);

regards,
dan carpenter

Re: [PATCH] mm/slab: make __free(kfree) accept error pointers

[Matthew Wilcox]

On Mon, Apr 29, 2024 at 09:29:58AM -0700, Christoph Lameter (Ampere) wrote:
> On Mon, 29 Apr 2024, Dan Carpenter wrote:
> 
> > I've always thought freeing pointers that have not been allocated is
> > sloppy so I like that kfree() doesn't allow error pointers.  We always
> > catch it before it reaches production and that teaches people better
> > habbits.  Personally, I like how free_netdev() only accepts valid
> > pointers.
> 
> kfree() already checks for NULL and ZERO pointers. We should add these
> checks in *one* location.
> 
> Maybe issue a WARN_ONCE() and simply treat it as a NULL pointer if an error
> code is passed?

Did you even read the initial patch?  The point is that this new automatic
destructor path can pass error pointers to the destructor for completely
valid code.  Warning would completely defeat the purpose of this exercise.

Re: [PATCH] mm/slab: make __free(kfree) accept error pointers

[Vlastimil Babka]

On 4/29/24 5:03 AM, Matthew Wilcox wrote:
> On Sun, Apr 28, 2024 at 05:26:44PM +0300, Dan Carpenter wrote:
>> Currently, if an automatically freed allocation is an error pointer that
>> will lead to a crash.  An example of this is in wm831x_gpio_dbg_show().
>> 
>>    171	char *label __free(kfree) = gpiochip_dup_line_label(chip, i);
>>    172	if (IS_ERR(label)) {
>>    173		dev_err(wm831x->dev, "Failed to duplicate label\n");
>>    174		continue;
>>    175  }
>> 
>> The auto clean up function should check for error pointers as well,
>> otherwise we're going to keep hitting issues like this.
>> 
>> Fixes: 54da6a092431 ("locking: Introduce __cleanup() based infrastructure")
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>> ---
>> Obviously, the fixes tag isn't very fair but it will tell the -stable
>> tools how far to backport this.
>> 
>>  include/linux/slab.h  | 4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/linux/slab.h b/include/linux/slab.h
>> index 4cc37ef22aae..5f5766219375 100644
>> --- a/include/linux/slab.h
>> +++ b/include/linux/slab.h
>> @@ -279,7 +279,7 @@ void kfree(const void *objp);
>>  void kfree_sensitive(const void *objp);
>>  size_t __ksize(const void *objp);
>>  
>> -DEFINE_FREE(kfree, void *, if (_T) kfree(_T))
>> +DEFINE_FREE(kfree, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T))
> 
> Wait, why do we check 'if (_T)' at all?  kfree() already handles NULL
> pointers just fine.  I wouldn't be averse to making it handle error
> pointers either.

Making kfree() handle IS_ERR() is perhaps a discussion for something else
than a stable fix. But Christoph has a point that kfree() checks
ZERO_OR_NULL_PTR. Here we check IS_ERR_OR_NULL. How about we checked only
IS_ERR here so it makes some sense?

>> -DEFINE_FREE(kvfree, void *, if (_T) kvfree(_T))
>> +DEFINE_FREE(kvfree, void *, if (!IS_ERR_OR_NULL(_T)) kvfree(_T))
> 
> Ditto kvfree().  Fixing kfree() would fix both of these.

ZERO and NULL should be both false for is_vmalloc_addr() so ultimately
kfree() will handle those, so we could also do only IS_ERR here?

Re: [PATCH] mm/slab: make __free(kfree) accept error pointers

[Dan Carpenter]

On Tue, Apr 30, 2024 at 02:09:10PM +0200, Vlastimil Babka wrote:
> On 4/29/24 5:03 AM, Matthew Wilcox wrote:
> > On Sun, Apr 28, 2024 at 05:26:44PM +0300, Dan Carpenter wrote:
> >> Currently, if an automatically freed allocation is an error pointer that
> >> will lead to a crash.  An example of this is in wm831x_gpio_dbg_show().
> >> 
> >>    171	char *label __free(kfree) = gpiochip_dup_line_label(chip, i);
> >>    172	if (IS_ERR(label)) {
> >>    173		dev_err(wm831x->dev, "Failed to duplicate label\n");
> >>    174		continue;
> >>    175  }
> >> 
> >> The auto clean up function should check for error pointers as well,
> >> otherwise we're going to keep hitting issues like this.
> >> 
> >> Fixes: 54da6a092431 ("locking: Introduce __cleanup() based infrastructure")
> >> Cc: <stable@vger.kernel.org>
> >> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> >> ---
> >> Obviously, the fixes tag isn't very fair but it will tell the -stable
> >> tools how far to backport this.
> >> 
> >>  include/linux/slab.h  | 4 ++--
> >>  1 files changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/include/linux/slab.h b/include/linux/slab.h
> >> index 4cc37ef22aae..5f5766219375 100644
> >> --- a/include/linux/slab.h
> >> +++ b/include/linux/slab.h
> >> @@ -279,7 +279,7 @@ void kfree(const void *objp);
> >>  void kfree_sensitive(const void *objp);
> >>  size_t __ksize(const void *objp);
> >>  
> >> -DEFINE_FREE(kfree, void *, if (_T) kfree(_T))
> >> +DEFINE_FREE(kfree, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T))
> > 
> > Wait, why do we check 'if (_T)' at all?  kfree() already handles NULL
> > pointers just fine.  I wouldn't be averse to making it handle error
> > pointers either.
> 
> Making kfree() handle IS_ERR() is perhaps a discussion for something else
> than a stable fix. But Christoph has a point that kfree() checks
> ZERO_OR_NULL_PTR. Here we check IS_ERR_OR_NULL. How about we checked only
> IS_ERR here so it makes some sense?
> 

I wondered why Peter Z wrote it like this as well...  I think he did
it so the compiler can figure out which calls to kfree() are unnecessary
and remove them.  These functions are inline and kfree() is not.  I
haven't measured to see if it actually results in a space savings but
the theory is sound.

regards,
dan carpenter

Re: [PATCH] mm/slab: make __free(kfree) accept error pointers

[Vlastimil Babka]

On 4/30/24 2:50 PM, Dan Carpenter wrote:
> On Tue, Apr 30, 2024 at 02:09:10PM +0200, Vlastimil Babka wrote:
>> On 4/29/24 5:03 AM, Matthew Wilcox wrote:
>> > On Sun, Apr 28, 2024 at 05:26:44PM +0300, Dan Carpenter wrote:
>> >> Currently, if an automatically freed allocation is an error pointer that
>> >> will lead to a crash.  An example of this is in wm831x_gpio_dbg_show().
>> >> 
>> >>    171	char *label __free(kfree) = gpiochip_dup_line_label(chip, i);
>> >>    172	if (IS_ERR(label)) {
>> >>    173		dev_err(wm831x->dev, "Failed to duplicate label\n");
>> >>    174		continue;
>> >>    175  }
>> >> 
>> >> The auto clean up function should check for error pointers as well,
>> >> otherwise we're going to keep hitting issues like this.
>> >> 
>> >> Fixes: 54da6a092431 ("locking: Introduce __cleanup() based infrastructure")
>> >> Cc: <stable@vger.kernel.org>
>> >> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>> >> ---
>> >> Obviously, the fixes tag isn't very fair but it will tell the -stable
>> >> tools how far to backport this.
>> >> 
>> >>  include/linux/slab.h  | 4 ++--
>> >>  1 files changed, 2 insertions(+), 2 deletions(-)
>> >> 
>> >> diff --git a/include/linux/slab.h b/include/linux/slab.h
>> >> index 4cc37ef22aae..5f5766219375 100644
>> >> --- a/include/linux/slab.h
>> >> +++ b/include/linux/slab.h
>> >> @@ -279,7 +279,7 @@ void kfree(const void *objp);
>> >>  void kfree_sensitive(const void *objp);
>> >>  size_t __ksize(const void *objp);
>> >>  
>> >> -DEFINE_FREE(kfree, void *, if (_T) kfree(_T))
>> >> +DEFINE_FREE(kfree, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T))
>> > 
>> > Wait, why do we check 'if (_T)' at all?  kfree() already handles NULL
>> > pointers just fine.  I wouldn't be averse to making it handle error
>> > pointers either.
>> 
>> Making kfree() handle IS_ERR() is perhaps a discussion for something else
>> than a stable fix. But Christoph has a point that kfree() checks
>> ZERO_OR_NULL_PTR. Here we check IS_ERR_OR_NULL. How about we checked only
>> IS_ERR here so it makes some sense?
>> 
> 
> I wondered why Peter Z wrote it like this as well...  I think he did
> it so the compiler can figure out which calls to kfree() are unnecessary
> and remove them.  These functions are inline and kfree() is not.  I
> haven't measured to see if it actually results in a space savings but
> the theory is sound.

Hmm that makes sense. There seem to be places that initialize the
__free(kfree) variable to NULL and only at some point actually allocate, and
between those there are possible returns, i.e. ice_init_hw().

OK, patch applied as-is to slab/for-6.9-rc7/fixes, thanks.

> regards,
> dan carpenter
>