Re: [PATCH v2] X.509: Introduce scope-based x509_certificate allocation

[Lukas Wunner <lukas@wunner.de>]

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