Received-SPF: pass (google.com: domain of linux-kernel+bounces-161774-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 29 Apr 2024 09:08:24 +0300
From: Dan Carpenter <dan.carpenter@linaro.org>
To: Matthew Wilcox <willy@infradead.org>
Cc: Christoph Lameter <cl@linux.com>, Peter Zijlstra <peterz@infradead.org>,
	Pekka Enberg <penberg@kernel.org>,
	David Rientjes <rientjes@google.com>,
	Joonsoo Kim <iamjoonsoo.kim@lge.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Vlastimil Babka <vbabka@suse.cz>,
	Roman Gushchin <roman.gushchin@linux.dev>,
	Hyeonggon Yoo <42.hyeyoo@gmail.com>, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org,
	Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Subject: Re: [PATCH] mm/slab: make __free(kfree) accept error pointers
Message-ID: <571761df-fe50-49e8-8d56-65fbdec9a185@moroto.mountain>
References: <285fee25-b447-47a1-9e00-3deb8f9af53e@moroto.mountain>
 <Zi8N66yehahl6D59@casper.infradead.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Zi8N66yehahl6D59@casper.infradead.org>

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