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

From: Dan Williams <dan.j.williams@intel.com>
To: Lukas Wunner <lukas@wunner.de>,
	David Howells <dhowells@redhat.com>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	"David S. Miller" <davem@davemloft.net>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: <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 v2] X.509: Introduce scope-based x509_certificate allocation
Date: Mon, 12 Feb 2024 11:07:06 -0800	[thread overview]
Message-ID: <65ca6c5ab2728_5a7f294fe@dwillia2-xfh.jf.intel.com.notmuch> (raw)
In-Reply-To: <4143b15418c4ecf87ddeceb36813943c3ede17aa.1707734526.git.lukas@wunner.de>

Lukas Wunner wrote:
> Jonathan suggests adding cleanup.h support for x509_certificate structs.
> cleanup.h is a newly introduced way to automatically free allocations at
> end of scope:  https://lwn.net/Articles/934679/
> 
> So add a DEFINE_FREE() clause for x509_certificate structs and use it in
> x509_cert_parse() and x509_key_preparse().  These are the only functions
> where scope-based x509_certificate allocation currently makes sense.
> A third user will be introduced with the forthcoming SPDM library
> (Security Protocol and Data Model) for PCI device authentication.
> 
> Unlike most other DEFINE_FREE() clauses, this one checks for IS_ERR()
> instead of NULL before calling x509_free_certificate() at end of scope.
> That's because the "constructor" of x509_certificate structs,
> x509_cert_parse(), returns a valid pointer or an ERR_PTR(), but never
> NULL.
> 
> I've compared the Assembler output before/after and they are identical,
> save for the fact that gcc-12 always generates two return paths when
> __cleanup() is used, one for the success case and one for the error case.
> 
> 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.
> 
> Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Link: https://lore.kernel.org/all/7721bfa3b4f8a99a111f7808ad8890c3c13df56d.1695921657.git.lukas@wunner.de/
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
> Changes v1 -> v2:
>  * Only check for !IS_ERR and not for NULL in DEFINE_FREE() clause (Herbert).
>    This avoids gratuitous NULL pointer checks at end of scope.
>    It's still safe to set a struct x509_certificate pointer to
>    NULL because x509_free_certificate() is a no-op in that case.
>  * Rephrase commit message (Jarkko):
>    Add Link tag, explain what cleanup.h is for, refer to DEFINE_FREE()
>    "clause" instead of "macro".
>  * Add assume() macro to <linux/compiler.h> and use it in x509_cert_parse()
>    to tell the compiler that kzalloc() never returns an ERR_PTR().
>    This avoids gratuitous IS_ERR() checks at end of scope.
> 
> Link to v1:
>  https://lore.kernel.org/all/70ecd3904a70d2b92f8f1e04365a2b9ce66fac25.1705857475.git.lukas@wunner.de/
> 
>  crypto/asymmetric_keys/x509_cert_parser.c | 43 ++++++++++++-------------------
>  crypto/asymmetric_keys/x509_parser.h      |  3 +++
>  crypto/asymmetric_keys/x509_public_key.c  | 31 +++++++---------------
>  include/linux/compiler.h                  |  2 ++
>  4 files changed, 30 insertions(+), 49 deletions(-)
> 
> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
> index 487204d..aeffbf6 100644
> --- a/crypto/asymmetric_keys/x509_cert_parser.c
> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -60,24 +60,24 @@ void x509_free_certificate(struct x509_certificate *cert)
>   */
>  struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
>  {
> -	struct x509_certificate *cert;
> -	struct x509_parse_context *ctx;
> +	struct x509_certificate *cert __free(x509_free_certificate);
> +	struct x509_parse_context *ctx __free(kfree) = NULL;
>  	struct asymmetric_key_id *kid;
>  	long ret;
>  
> -	ret = -ENOMEM;
>  	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.