Re: [PATCH v3] X.509: Introduce scope-based x509_certificate allocation

From: Lukas Wunner <lukas@wunner.de>
To: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David Howells <dhowells@redhat.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	keyrings@vger.kernel.org, linux-crypto@vger.kernel.org,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Dan Williams <dan.j.williams@intel.com>,
	Ard Biesheuvel <ardb@kernel.org>,
	Jarkko Sakkinen <jarkko@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Nathan Chancellor <nathan@kernel.org>
Subject: Re: [PATCH v3] X.509: Introduce scope-based x509_certificate allocation
Date: Sat, 2 Mar 2024 09:27:51 +0100	[thread overview]
Message-ID: <20240302082751.GA25828@wunner.de> (raw)
In-Reply-To: <ZeGpmbawHkLNcwFy@gondor.apana.org.au>

On Fri, Mar 01, 2024 at 06:10:33PM +0800, Herbert Xu wrote:
> On Tue, Feb 20, 2024 at 04:10:39PM +0100, Lukas Wunner wrote:
> >
> > In x509_cert_parse(), add a hint for the compiler that kzalloc() never
> > returns an ERR_PTR().  Otherwise the compiler adds a gratuitous IS_ERR()
> > check on return.  Introduce a handy assume() macro for this which can be
> > re-used elsewhere in the kernel to provide hints for the compiler.
> 
> Would it be possible to move the use of assume into the kzalloc
> declaration instead? Perhaps by turning it into a static inline
> wrapper that does the "assume"?
> 
> Otherwise as time goes on we'll have a proliferation of these
> "assume"s all over the place.

I've tried moving the assume(!IS_ERR()) to kmalloc() (which already is
a static inline), but that increased total vmlinux size by 448 bytes.
I was expecting pushback due to the size increase, hence kept the
assume() local to x509_cert_parse().

There's a coccinelle rule which warns if an IS_ERR() check is performed
on a kmalloc'ed pointer (scripts/coccinelle/null/eno.cocci), hence there
don't seem to be any offenders left in the tree which use this antipattern
and adding assume(!IS_ERR()) to kmalloc() doesn't have any positive effect
beyond avoiding the single unnecessary check in x509_cert_parse().

If you don't like the assume(!IS_ERR(cert)) in x509_cert_parse(),
I can respin the patch to drop it.  The unnecessary check which it
avoids only occurs in the error path.  If the certificate can be
parsed without error, there's no unnecessary check.  It still
triggered my OCD when scrutinizing the disassembled code and
sufficiently annoyed me that I wanted to get rid of it,
but in reality it's not such a big deal.

Thanks,

Lukas