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 <Jonathan.Cameron@xxxxxxxxxx> > Link: https://lore.kernel.org/all/7721bfa3b4f8a99a111f7808ad8890c3c13df56d.1695921657.git.lukas@xxxxxxxxx/ > Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx> > --- > 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@xxxxxxxxx/ > > 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.
