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>, 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 12:57:02 -0800	[thread overview]
Message-ID: <65ca861e14779_5a7f2949e@dwillia2-xfh.jf.intel.com.notmuch> (raw)
In-Reply-To: <20240212192009.GA13884@wunner.de>

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) \
