Re: [PATCH v5 5/8] virtio: Treat alloc_dax() -EOPNOTSUPP failure as non-fatal

[Dan Williams <dan.j.williams@intel.com>]

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.