Lukas Wunner wrote:
> On Mon, Feb 12, 2024 at 03:02:46PM -0800, Dan Williams wrote:
> > However, Lukas, I think Linus is right, your DEFINE_FREE() should use
> > IS_ERR_OR_NULL().
>
> Uh... that's a negative, sir. ;)
>
> IS_ERR_OR_NULL() results in...
> * a superfluous NULL pointer check in x509_key_preparse() and
> * a superfluous IS_ERR check in x509_cert_parse().
>
> IS_ERR() results *only* in...
> * a superfluous IS_ERR check in x509_cert_parse().
>
> I can get rid of the IS_ERR() check by using assume().
>
> I can *not* get rid of the NULL pointer check because the compiler
> is compiled with -fno-delete-null-pointer-checks. (The compiler
> seems to ignore __attribute__((returns_nonnull)) due to that.)
>
>
> > I.e. the problem is trying to use
> > __free(x509_free_certificate) in x509_cert_parse().
> >
> > > --- 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);
> >
> > ...make this:
> >
> > struct x509_certificate *cert __free(kfree);
>
> That doesn't work I'm afraid. x509_cert_parse() needs
> x509_free_certificate() to be called in the error path,
> not kfree(). See the existing code in current mainline:
>
> x509_cert_parse() populates three sub-allocations in
> struct x509_certificate (pub, sig, id) and two
> sub-sub-allocations (pub->key, pub->params).
>
> So I'd have to add five additional local variables which
> get freed by __cleanup(). One of them (pub->key) requires
> kfree_sensitive() instead of kfree(), so I'd need an extra
> DEFINE_FREE() for that.
>
> I haven't tried it but I suspect the result would look
> terrible and David Howells wouldn't like it.
Ugh, that's what I was afraid of, so these cases are different.
> > ...and Mathieu, this should be IS_ERR_OR_NULL() to skip an unnecessary
> > call to virtio_fs_cleanup_dax() at function exit that the compiler
> > should elide.
>
> My recommendation is to check for !IS_ERR() in the DEFINE_FREE() clause
> and amend virtio_fs_cleanup_dax() with a "if (!dax_dev) return;" for
> defensiveness in case someone calls it with a NULL pointer.
The internal calls (kill_dax(), put_dax()) check for NULL already, so I
don't think that's needed.
> That's the best solution I could come up with for the x509_certificate
> conversion.
>
> Note that even with superfluous checks avoided, __cleanup() causes
> gcc-12 to always generate two return paths. It's very visible in
> the generated code that all the stack unwinding code gets duplicated
> in every function using __cleanup(). The existing Assembler code
> of x509_key_preparse() and x509_cert_parse(), without __cleanup()
> invocation, has only a single return path.
I saw that too, some NULL checks can indeed be elided with a NULL check
in the DEFINE_FREE(), but the multiple exit paths still someimtes result
in __cleanup() using functions being larger than the goto equivalent.
> So __cleanup() bloats the code regardless of superfluous checks,
> but future gcc versions might avoid that. clang-15 generates much
> more compact code (vmlinux is a couple hundred kBytes smaller),
> but does weird things such as inlining x509_free_certificate()
> in x509_cert_parse().
>
> As you may have guessed, I've spent an inordinate amount of time
> down that rabbit hole. ;(
Hey, this is new and interesting stuff, glad we are grappling with it at
this level.
