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

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: <571761df-fe50-49e8-8d56-65fbdec9a185@moroto.mountain>
Date: Mon, 29 Apr 2024 09:08:24 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Matthew Wilcox <willy@...radead.org>
Cc: Christoph Lameter <cl@...ux.com>, Peter Zijlstra <peterz@...radead.org>,
	Pekka Enberg <penberg@...nel.org>,
	David Rientjes <rientjes@...gle.com>,
	Joonsoo Kim <iamjoonsoo.kim@....com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Vlastimil Babka <vbabka@...e.cz>,
	Roman Gushchin <roman.gushchin@...ux.dev>,
	Hyeonggon Yoo <42.hyeyoo@...il.com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH] mm/slab: make __free(kfree) accept error pointers

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