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 <[email protected]>
Link: https://lore.kernel.org/all/7721bfa3b4f8a99a111f7808ad8890c3c13df56d.1695921657.git.lukas@wunner.de/
Signed-off-by: Lukas Wunner <[email protected]>
---
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 */
if (!cert)
- goto error_no_cert;
+ return ERR_PTR(-ENOMEM);
cert->pub = kzalloc(sizeof(struct public_key), GFP_KERNEL);
if (!cert->pub)
- goto error_no_ctx;
+ return ERR_PTR(-ENOMEM);
cert->sig = kzalloc(sizeof(struct public_key_signature), GFP_KERNEL);
if (!cert->sig)
- goto error_no_ctx;
+ return ERR_PTR(-ENOMEM);
ctx = kzalloc(sizeof(struct x509_parse_context), GFP_KERNEL);
if (!ctx)
- goto error_no_ctx;
+ return ERR_PTR(-ENOMEM);
ctx->cert = cert;
ctx->data = (unsigned long)data;
@@ -85,7 +85,7 @@ struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
/* Attempt to decode the certificate */
ret = asn1_ber_decoder(&x509_decoder, ctx, data, datalen);
if (ret < 0)
- goto error_decode;
+ return ERR_PTR(ret);
/* Decode the AuthorityKeyIdentifier */
if (ctx->raw_akid) {
@@ -95,20 +95,19 @@ struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
ctx->raw_akid, ctx->raw_akid_size);
if (ret < 0) {
pr_warn("Couldn't decode AuthKeyIdentifier\n");
- goto error_decode;
+ return ERR_PTR(ret);
}
}
- ret = -ENOMEM;
cert->pub->key = kmemdup(ctx->key, ctx->key_size, GFP_KERNEL);
if (!cert->pub->key)
- goto error_decode;
+ return ERR_PTR(-ENOMEM);
cert->pub->keylen = ctx->key_size;
cert->pub->params = kmemdup(ctx->params, ctx->params_size, GFP_KERNEL);
if (!cert->pub->params)
- goto error_decode;
+ return ERR_PTR(-ENOMEM);
cert->pub->paramlen = ctx->params_size;
cert->pub->algo = ctx->key_algo;
@@ -116,33 +115,23 @@ struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
/* Grab the signature bits */
ret = x509_get_sig_params(cert);
if (ret < 0)
- goto error_decode;
+ return ERR_PTR(ret);
/* Generate cert issuer + serial number key ID */
kid = asymmetric_key_generate_id(cert->raw_serial,
cert->raw_serial_size,
cert->raw_issuer,
cert->raw_issuer_size);
- if (IS_ERR(kid)) {
- ret = PTR_ERR(kid);
- goto error_decode;
- }
+ if (IS_ERR(kid))
+ return ERR_CAST(kid);
cert->id = kid;
/* Detect self-signed certificates */
ret = x509_check_for_self_signed(cert);
if (ret < 0)
- goto error_decode;
-
- kfree(ctx);
- return cert;
+ return ERR_PTR(ret);
-error_decode:
- kfree(ctx);
-error_no_ctx:
- x509_free_certificate(cert);
-error_no_cert:
- return ERR_PTR(ret);
+ return_ptr(cert);
}
EXPORT_SYMBOL_GPL(x509_cert_parse);
diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
index 97a886c..0688c22 100644
--- a/crypto/asymmetric_keys/x509_parser.h
+++ b/crypto/asymmetric_keys/x509_parser.h
@@ -5,6 +5,7 @@
* Written by David Howells ([email protected])
*/
+#include <linux/cleanup.h>
#include <linux/time.h>
#include <crypto/public_key.h>
#include <keys/asymmetric-type.h>
@@ -44,6 +45,8 @@ struct x509_certificate {
* x509_cert_parser.c
*/
extern void x509_free_certificate(struct x509_certificate *cert);
+DEFINE_FREE(x509_free_certificate, struct x509_certificate *,
+ if (!IS_ERR(_T)) x509_free_certificate(_T))
extern struct x509_certificate *x509_cert_parse(const void *data, size_t datalen);
extern int x509_decode_time(time64_t *_t, size_t hdrlen,
unsigned char tag,
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 6a4f00b..00ac715 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -161,12 +161,11 @@ int x509_check_for_self_signed(struct x509_certificate *cert)
*/
static int x509_key_preparse(struct key_preparsed_payload *prep)
{
- struct asymmetric_key_ids *kids;
- struct x509_certificate *cert;
+ struct x509_certificate *cert __free(x509_free_certificate);
+ struct asymmetric_key_ids *kids __free(kfree) = NULL;
+ char *p, *desc __free(kfree) = NULL;
const char *q;
size_t srlen, sulen;
- char *desc = NULL, *p;
- int ret;
cert = x509_cert_parse(prep->data, prep->datalen);
if (IS_ERR(cert))
@@ -188,9 +187,8 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
}
/* Don't permit addition of blacklisted keys */
- ret = -EKEYREJECTED;
if (cert->blacklisted)
- goto error_free_cert;
+ return -EKEYREJECTED;
/* Propose a description */
sulen = strlen(cert->subject);
@@ -202,10 +200,9 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
q = cert->raw_serial;
}
- ret = -ENOMEM;
desc = kmalloc(sulen + 2 + srlen * 2 + 1, GFP_KERNEL);
if (!desc)
- goto error_free_cert;
+ return -ENOMEM;
p = memcpy(desc, cert->subject, sulen);
p += sulen;
*p++ = ':';
@@ -215,16 +212,14 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
kids = kmalloc(sizeof(struct asymmetric_key_ids), GFP_KERNEL);
if (!kids)
- goto error_free_desc;
+ return -ENOMEM;
kids->id[0] = cert->id;
kids->id[1] = cert->skid;
kids->id[2] = asymmetric_key_generate_id(cert->raw_subject,
cert->raw_subject_size,
"", 0);
- if (IS_ERR(kids->id[2])) {
- ret = PTR_ERR(kids->id[2]);
- goto error_free_kids;
- }
+ if (IS_ERR(kids->id[2]))
+ return PTR_ERR(kids->id[2]);
/* We're pinning the module by being linked against it */
__module_get(public_key_subtype.owner);
@@ -242,15 +237,7 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
cert->sig = NULL;
desc = NULL;
kids = NULL;
- ret = 0;
-
-error_free_kids:
- kfree(kids);
-error_free_desc:
- kfree(desc);
-error_free_cert:
- x509_free_certificate(cert);
- return ret;
+ return 0;
}
static struct asymmetric_key_parser x509_key_parser = {
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)
+
/*
* KENTRY - kernel entry point
* This can be used to annotate symbols (functions or data) that are used
--
2.43.0
On Mon, Feb 12, 2024 at 12:24:39PM +0100, 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.
Shouldn't it be in a separate patch?
...
> +#define assume(cond) do if(!(cond)) __builtin_unreachable(); while(0)
Missing spaces? Missing braces (for the sake of robustness)?
#define assume(cond) do { if (!(cond)) __builtin_unreachable(); } while (0)
--
With Best Regards,
Andy Shevchenko
On Mon, Feb 12, 2024 at 02:19:00PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 12, 2024 at 12:24:39PM +0100, 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.
>
> Shouldn't it be in a separate patch?
The advantage of introducing it in this patch is that someone later
examining the git history with "git blame" + "git log" will directly
see why exactly it was added and what it's good for. Often people
introduce a feature in one patch but its usage is in a different patch
and that means more digging in the git history, which can be annoying.
I also don't see an *advantage* of splitting into two patches. If someone
decides to revert the DEFINE_FREE() conversion for x509_certificate structs,
they would leave the assume() macro behind because it was in a separate
patch. Leaving an unused macro behind should probably be avoided.
Granted if at that point there are additional assume() users, the revert
patch would have to be edited, but who knows if and when those will appear.
> > +#define assume(cond) do if(!(cond)) __builtin_unreachable(); while(0)
>
> Missing spaces? Missing braces (for the sake of robustness)?
>
> #define assume(cond) do { if (!(cond)) __builtin_unreachable(); } while (0)
Hm, I'm not sure why this improves robustness?
Readability might be an argument for the braces. *shrug*
Thanks,
Lukas
On Mon Feb 12, 2024 at 1:24 PM EET, Lukas Wunner wrote:
> Jonathan suggests adding cleanup.h support for x509_certificate structs.
Please remove as this is just repeating suggested-by tag.
> cleanup.h is a newly introduced way to automatically free allocations at
> end of scope: https://lwn.net/Articles/934679/
cleanup.h is not a feature, it is a header file.
Use link tag for LWN and I guess the feature is called scoped based
resource maangement.
BR, Jarkko
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 <[email protected]>
> Link: https://lore.kernel.org/all/7721bfa3b4f8a99a111f7808ad8890c3c13df56d.1695921657.git.lukas@wunner.de/
> Signed-off-by: Lukas Wunner <[email protected]>
> ---
> 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.
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) \