Received-SPF: pass (google.com: domain of linux-crypto+bounces-2006-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 12 Feb 2024 12:57:02 -0800
From: Dan Williams <dan.j.williams@intel.com>
To: Lukas Wunner <lukas@wunner.de>, 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
Message-ID: <65ca861e14779_5a7f2949e@dwillia2-xfh.jf.intel.com.notmuch>
References: <4143b15418c4ecf87ddeceb36813943c3ede17aa.1707734526.git.lukas@wunner.de>
 <65ca6c5ab2728_5a7f294fe@dwillia2-xfh.jf.intel.com.notmuch>
 <20240212192009.GA13884@wunner.de>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240212192009.GA13884@wunner.de>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?bq/hZbtYylofF6rSnhrcal9UY+Ngae+VJgKiySPeaAZv0CXyhYKmQGSZShSB?=
 =?us-ascii?Q?MWgDYq3S8vOBn/isIxEej0U08rmK7o36bftuwdCYN//mFVQw5831NcDc5nfS?=
 =?us-ascii?Q?De/vUeX2CV6hPZwLvMteDoOw7upXVUOBYBOTuk12ZTR3BvkGaT6HI1ELFKHq?=
 =?us-ascii?Q?tJDn6Yt/iUPSCH6qxEc5bs2X3QripPi1tSaoga7AZPHSy4mgokY2CnYjFObP?=
 =?us-ascii?Q?eIN68YytWC4v5YFshsesN8NV14W0HTYbHmiGnofA4HM18TxyuIIMMxfVKeVW?=
 =?us-ascii?Q?5ySV1ynCo8QLx2A8k+clPITPcYHi63R51iSIizHjhixfhSrXP2gkkSutZU/n?=
 =?us-ascii?Q?Ds9evA0A1KeveiUQ7swZmjxlac4QCCtPCvbcT/a0MqtXo4eh8g68Dc1ZNOQW?=
 =?us-ascii?Q?t/2uVtpTKurrekyBH/h1geR8cvXDNRrerCm9AJZG1G4aVgOSx6gyqR0l40uF?=
 =?us-ascii?Q?XE7O/uHV3jXAWh/gzASNvdk+1bV53DWusZfUcTX5W/6rBmH7hTlnCZ9r8C3h?=
 =?us-ascii?Q?Suz+7LUuNAcVpe/wgWX421L20aHFoEyOthyreROl2G73HqvF40q6TDLbf2V5?=
 =?us-ascii?Q?VBy3boJkAhPcuz42xhsNyH8hp961r41SKFvM0Qq54bEqjx1thesukctY+WWG?=
 =?us-ascii?Q?I0MViDNVVs63llHCGamABTB9Kfm9YOQUA0HCBZezCGtl2DpDx4P+9BIYwflo?=
 =?us-ascii?Q?mMdpwLAdUYJ4WjGyoe8vjAGHxzaT3BB84dB8lAo5EKiAj5xsE+kAmjbby9SH?=
 =?us-ascii?Q?g4zjjIqf68Rga86Ha69E3FnxG/Aie2cT4v2olB5OKo0T79c/HIBt4pG5xR9L?=
 =?us-ascii?Q?+WzV3zMpY/1mvFfRxu0jfH9BCLTRrmnjf6/4IZxiwZhqJ6ItjzCF4woWgmAW?=
 =?us-ascii?Q?WbNbeWT1mc89fCT9b7qGFdzp3A1Cuv++H9iXNIQlDmTIaM00GzkEnUSrmuNY?=
 =?us-ascii?Q?tvGNAVu5SwEHmzrkhTKyowu5ANOHLe++ZOs1pNaeWYG26lgtbi0ZtVpsB8bv?=
 =?us-ascii?Q?SNdUAzEK3naWaYhAylPiOCpk2Mlhx7XFu5o1Lh3Mqa2YaFa3NbYJ7hHQPOvt?=
 =?us-ascii?Q?UtQx/Qm/x4n+CjIaNllQnpmG6xXnCK80j+4Iw2vlPPM1HNJGEaeAWdmxhnSI?=
 =?us-ascii?Q?QvFX4qWdEcLFo2UM9RHa2SuzS52u/Bj+qvdKquCLzzk3NNQRjO5cFitucbk2?=
 =?us-ascii?Q?QIVn6XIpgOiWXu1wP+doZYTYDllqNqp5c9wW8i97STtROYJXpAmzVT6DQ1Ko?=
 =?us-ascii?Q?HAxx3r7j//ArZqfo/mmlkN7wo18e3YtKzXipghyOhrXfyZJC88QtsBo0ANuK?=
 =?us-ascii?Q?uURNa8EvGHBSzTYZb/KE6XedtOSWMpc+7urK+SncC60cqhv/CSwGapuTTmOD?=
 =?us-ascii?Q?L7yVtg+pNTHUEFUeOvkZ+J6AWHX/oQtq1E3YDEA4AqRDdJxlFrMZyvvWSUQ8?=
 =?us-ascii?Q?TYyWKTTAu0/YYBZLURfWoHXjIagP6ZyDoQbuLTqwEhNz/7SicE9iiUXqdyYT?=
 =?us-ascii?Q?EM3JoGF5Lg6Na6gy6PNzVvfNXvZc9jXp2hThQWwQTQo7ODs7n0H0lG5Xy1Lt?=
 =?us-ascii?Q?y9m8e2cClT25HhMVZ1oxWBkoAJ5e6sBwB4rqtPMJn5AkyK1EYgJEyjMbtPPL?=
 =?us-ascii?Q?xw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 7bc6f64f-8ff4-43c8-cb58-08dc2c0d2b2c
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Feb 2024 20:57:05.1874
 (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: gdkvg+tGfKIW9UoALYH7VPvNVGG6OWIxzkBFDLeYTMgRkM3KObwlUa/ziGlriEKCaSXDcAMZtzyCkuCJlVdyzoXBKh3NLgt3iax4Egw705Q=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR11MB4665
X-OriginatorOrg: intel.com

Lukas Wunner wrote:
> 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.

Ah, ok, and I now I see you already addressed that in the changelog for
v1, but dropped that comment for v2. Might I suggest the following:

> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index bb1339c..384803e 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -139,6 +139,8 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
>  } while (0)
>  #endif
>  
> +#define assume(cond) do if(!(cond)) __builtin_unreachable(); while(0)

s/__builtin_unreachable()/unreachable()/?

Move this to cleanup.h and add extend the DEFINE_FREE() comment about
its usage:

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index c2d09bc4f976..b4380a69ac72 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -56,6 +56,32 @@
  *	return p;
  *
  * Without the NULL test it turns into a mess and the compiler can't help us.
+ *
+ * NOTE2: For scoped based resource management in paths that want to
+ * cleanup an allocation *and* return an ERR_PTR() on failure, the
+ * compiler needs more help. Use an IS_ERR() check in the DEFINE_FREE()
+ * definition and then use assume() to tell the compiler that the
+ * allocation never returns an ERR_PTR().
+ *
+ * Ex.
+ *
+ * DEFINE_FREE(free_obj, struct obj *, if (!IS_ERR(_T)) free_obj(_T))
+ *
+ * struct obj *create_obj(...)
+ * {
+ *	struct obj *p __free(free_obj) = kzalloc(...);
+ *	int rc;
+ *
+ *	assume(!IS_ERR(p));
+ *	if (!p)
+ *		return ERR_PTR(-ENOMEM);
+ *
+ *	rc = init_obj(p);
+ *	if (rc)
+ *		return PTR_ERR(rc);
+ *
+ *	return_ptr(p);
+ * }
  */
 
 #define DEFINE_FREE(_name, _type, _free) \