| Message ID | 285fee25-b447-47a1-9e00-3deb8f9af53e@moroto.mountain (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/slab: make __free(kfree) accept error pointers | expand |
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>
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.
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
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.
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?
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
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 >
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);
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(-)