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