| Message ID | 63cc7ab17a5064756e26e50bc605e3ff8914f05a.1708439875.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [v3] X.509: Introduce scope-based x509_certificate allocation | expand |
On Tue Feb 20, 2024 at 3:10 PM UTC, Lukas Wunner wrote: > 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. I think you are adding scope-based memory management and not DEFINE_FREE(). Otherwise, this would be one-liner patch. I'm not sure if the last sentence adds more than clutter as this patch has nothing to do with SPDM changes per se. > 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. Use passive as commit message is not a personal letter. > > 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. Does not explain why it is "handy". I don't see a story here but instead I see bunch of disordered tecnical terms. We have the code diff for detailed technical stuff. The commit message should simply explain why we want this and what it does for us. And we zero care about PCI changes in the scope of this patch, especially since this is not part of such patch set. BR, Jarkko
On Tue Feb 20, 2024 at 6:00 PM UTC, Jarkko Sakkinen wrote: > On Tue Feb 20, 2024 at 3:10 PM UTC, Lukas Wunner wrote: > > 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. > > I think you are adding scope-based memory management and not > DEFINE_FREE(). Otherwise, this would be one-liner patch. > > I'm not sure if the last sentence adds more than clutter as this > patch has nothing to do with SPDM changes per se. > > > 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. > > Use passive as commit message is not a personal letter. > > > > > 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. > > Does not explain why it is "handy". > > I don't see a story here but instead I see bunch of disordered tecnical > terms. > > We have the code diff for detailed technical stuff. The commit message > should simply explain why we want this and what it does for us. And we > zero care about PCI changes in the scope of this patch, especially since > this is not part of such patch set. I mean think it this way. What is the most important function of a commit message? Well, it comes when the commit is in the mainline. It reminds of the *reasons* why a change was made and this commit message does not really serve well in that role. BR, Jarkko
On Tue, Feb 20, 2024 at 04:10:39PM +0100, 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. Would it be possible to move the use of assume into the kzalloc declaration instead? Perhaps by turning it into a static inline wrapper that does the "assume"? Otherwise as time goes on we'll have a proliferation of these "assume"s all over the place. Thanks,
On Fri, Mar 01, 2024 at 06:10:33PM +0800, Herbert Xu wrote: > On Tue, Feb 20, 2024 at 04:10:39PM +0100, 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. > > Would it be possible to move the use of assume into the kzalloc > declaration instead? Perhaps by turning it into a static inline > wrapper that does the "assume"? > > Otherwise as time goes on we'll have a proliferation of these > "assume"s all over the place. I've tried moving the assume(!IS_ERR()) to kmalloc() (which already is a static inline), but that increased total vmlinux size by 448 bytes. I was expecting pushback due to the size increase, hence kept the assume() local to x509_cert_parse(). There's a coccinelle rule which warns if an IS_ERR() check is performed on a kmalloc'ed pointer (scripts/coccinelle/null/eno.cocci), hence there don't seem to be any offenders left in the tree which use this antipattern and adding assume(!IS_ERR()) to kmalloc() doesn't have any positive effect beyond avoiding the single unnecessary check in x509_cert_parse(). If you don't like the assume(!IS_ERR(cert)) in x509_cert_parse(), I can respin the patch to drop it. The unnecessary check which it avoids only occurs in the error path. If the certificate can be parsed without error, there's no unnecessary check. It still triggered my OCD when scrutinizing the disassembled code and sufficiently annoyed me that I wanted to get rid of it, but in reality it's not such a big deal. Thanks, Lukas
On Sat, Mar 02, 2024 at 09:27:51AM +0100, Lukas Wunner wrote: > > I've tried moving the assume(!IS_ERR()) to kmalloc() (which already is > a static inline), but that increased total vmlinux size by 448 bytes. > I was expecting pushback due to the size increase, hence kept the > assume() local to x509_cert_parse(). OK if you've already tried it then I'll take this as it stands. Thanks,
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 (dhowells@redhat.com) */ +#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..aee74ba 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
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/20231003153937.000034ca@Huawei.com/ Link: https://lwn.net/Articles/934679/ Signed-off-by: Lukas Wunner <lukas@wunner.de> --- Changes v2 -> v3: * Add curly braces to "do { } while" loop in assume() macro (Andy). * Rephrase commit message: Drop first paragraph, use "Link:" tag for LWN article (Jarkko). Link to v2: https://lore.kernel.org/all/4143b15418c4ecf87ddeceb36813943c3ede17aa.1707734526.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(-)