Received-SPF: pass (google.com: domain of linux-crypto+bounces-2004-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 12 Feb 2024 11:07:06 -0800
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
Message-ID: <65ca6c5ab2728_5a7f294fe@dwillia2-xfh.jf.intel.com.notmuch>
References: <4143b15418c4ecf87ddeceb36813943c3ede17aa.1707734526.git.lukas@wunner.de>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <4143b15418c4ecf87ddeceb36813943c3ede17aa.1707734526.git.lukas@wunner.de>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?L/9ChKKhhngJ4VmmPDkkX8OgCvKxYZh1nWKdz+R0uW6sh9N057JxafvA2ix0?=
 =?us-ascii?Q?gbTf4SRLL590yPYT+nhhDHhLar6wQDaV0yP9evkmj1t41VWGHlC7DEQkIxN9?=
 =?us-ascii?Q?7a1OlmHH9BuDwNfumqWYv/uoZHuydJybASpGE6makxWiTqRJXdcbstMeCNCb?=
 =?us-ascii?Q?gUf8IsfexjLczaK5KNUJLtO6WmlKpqhe7nst6nXF8M8yWcfoWWExYAxFs/Ct?=
 =?us-ascii?Q?U/ZZM7pvaqWT4++t3CFHLuoJKKqqiUlSL3/2uZ/LAEnaS5CaO41RdvDsETas?=
 =?us-ascii?Q?tZmFor/4p0g2ULX/vZLcB/7wo5LSFWxV04ICUPi4ZXKqvFZGKVcYQS1y8v+A?=
 =?us-ascii?Q?7TgSa87AfKS6n5BbiH6dNfs9t7Erge3iEbI7q6rW7kG6zjnO1CqG36lTmktH?=
 =?us-ascii?Q?vWg4A5zwwXsE/WwJbhL4BPIiC4pINH0epH2Tw45zuBOspz+hPuB0DpgGcE9k?=
 =?us-ascii?Q?XD8SeJqrhg1otki6OVBnnwODB/AdfIKYd5PRTcqSy3ul8/8s+7YFH4e28XO/?=
 =?us-ascii?Q?bo+UArogGI2/auCbb0Mm3R6vf+F2bglkXaspUVmpJodN9tibSl73PYlhgF0i?=
 =?us-ascii?Q?VtuXcCZWjvkBnyHzzaa/WVsqLDTwjBAxY/Cl9LGKZ6tTpydi7ngaly0pcM64?=
 =?us-ascii?Q?V3LrrYAmrOIyfu8NbEDh5z68AeK8vFVeCIdl/KiKjLpqIoTq1DoElOi14Jt6?=
 =?us-ascii?Q?B19sbe/68byshzAAyElEsW1sQSjmzS5TJZJXuF3C8OshM1YI56NZvXMVTH9S?=
 =?us-ascii?Q?6efrNZKxIqoQ3QziBMfpETrZBg6doBEZI8QKT+pBYkGir2zYXgDua1n+Mudz?=
 =?us-ascii?Q?LMYvMjLuVCKOSLLBJIZ4t3nwuCaSqa9cAoEQTEdVYDdFEJNyvB7Vzzr046Hn?=
 =?us-ascii?Q?MUoTf2aW+3FzEX/gIuqbzJHxHd4Zu8xw2WeJkw0lzLQE50ANTGonwq9UHyiX?=
 =?us-ascii?Q?qkhNMYtdATIjo5Rmk0nFvC0O5+kCQXUPkCve5LIsDVktPQbumDzxfyS9xfQM?=
 =?us-ascii?Q?LkkG1MpTGBBgYKILPS7HANx6emrzGz97FPHaIYaCJOhBo+7S/+u2/8m2WArD?=
 =?us-ascii?Q?NhUtTvhvbYSnn2AM+lFzcB2IpN4LnleoEUFBOjnwKriZs4Z2PhBVve0iQJKo?=
 =?us-ascii?Q?BhyiZ2hAU0VehgpdNQIyGBt2yipuVHwtzl+l+eOyhEHsozeLeubhy2d5SVuP?=
 =?us-ascii?Q?h1srALvOYhblohcqjXLDuFbR31G1Q2r6XLBtcBcKzveNzDXl6JcAukdI3drU?=
 =?us-ascii?Q?Ke3m4ydZX0T/NR5nHX6jJEznd9r2moSBYlxtw6quKwyZQi2P9E40W/FS7hVd?=
 =?us-ascii?Q?BytYQNW9yUE4CsxCHmjuo3OUA/hYCgPojuqYY2/ip1AJ6TpOW/BnyxlzO60F?=
 =?us-ascii?Q?c1FekkLLZF8ZhO10xIZvCH8itPG2719i9/du8QpIdhbNyLmcSl29cPSg7eOn?=
 =?us-ascii?Q?ClsI6ePqSrCnJcXQJFmJUfHUxOkxg5+THJCSbib9eQeZrOYjwDs30XMgj9A6?=
 =?us-ascii?Q?ixETjGE+orkH3lY3Q6Z0srnyOwUr9qYUWa/wgEMZfXZgSAEvG3uqzThsPAFT?=
 =?us-ascii?Q?NopDHtXrIK3jdfI+G/oe75WIR42ox2otQHO7cVi4jGXWkyiTxl0HPuFRMzzb?=
 =?us-ascii?Q?Og=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5d7c41a3-b31d-4e8c-ef12-08dc2bfdcfba
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Feb 2024 19:07:09.3217
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: yDsFomPOvFGkbnyON/zizG8W+CsggV0aOpGezA8KWTdJnGI/p0hRxewf1aWE3g/UeEWlcTBbEu7agBiBG3IIl/hrPvXZjo7HrU1MGpfdsww=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5369
X-OriginatorOrg: intel.com

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.