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

From: Lukas Wunner <lukas@wunner.de>
To: Dan Williams <dan.j.williams@intel.com>
Cc: David Howells <dhowells@redhat.com>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	"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>,
	Ard Biesheuvel <ardb@kernel.org>,
	Jarkko Sakkinen <jarkko@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Nathan Chancellor <nathan@kernel.org>
Subject: Re: [PATCH v2] X.509: Introduce scope-based x509_certificate allocation
Date: Mon, 12 Feb 2024 20:20:09 +0100	[thread overview]
Message-ID: <20240212192009.GA13884@wunner.de> (raw)
In-Reply-To: <65ca6c5ab2728_5a7f294fe@dwillia2-xfh.jf.intel.com.notmuch>

On Mon, Feb 12, 2024 at 11:07:06AM -0800, Dan Williams wrote:
> 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.
[...]
> >  	cert = kzalloc(sizeof(struct x509_certificate), GFP_KERNEL);
> > +	assume(!IS_ERR(cert)); /* Avoid gratuitous IS_ERR() check on return */
> 
> I like the idea of assume() I just wonder if it should move inside of
> the kmalloc() inline definition itself? I.e. solve the "cleanup.h" vs
> ERR_PTR() rough edge more generally.

I've tried that but total vmlinux size increased by 448 bytes.
It seems to cause additional code or padding somewhere.  To avoid
pushback because of that, I confined it to just x509_cert_parse().

I should mention that there's a coccinelle rule which warns if
someone performs an IS_ERR() check on a kmalloc'ed pointer
(scripts/coccinelle/null/eno.cocci).  Which is why there likely
aren't any offenders in the tree.  That rule is triggered by
this assume() clause, but it's obviously a false-positive.
I'll look into suppressing that warning if/when this patch
gets accepted.

I should also mention that assume() currently only has an effect
with gcc.  clang-15 just ignored it during my testing.

Thanks,

Lukas