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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <65cbbb6d24b71_5c7629450@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Date: Tue, 13 Feb 2024 10:56:45 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Lukas Wunner <lukas@...ner.de>, Dan Williams <dan.j.williams@...el.com>
CC: Linus Torvalds <torvalds@...ux-foundation.org>, Mathieu Desnoyers
	<mathieu.desnoyers@...icios.com>, Arnd Bergmann <arnd@...db.de>, Dave Chinner
	<david@...morbit.com>, <linux-kernel@...r.kernel.org>, Andrew Morton
	<akpm@...ux-foundation.org>, Vishal Verma <vishal.l.verma@...el.com>, "Dave
 Jiang" <dave.jiang@...el.com>, Matthew Wilcox <willy@...radead.org>, "Russell
 King" <linux@...linux.org.uk>, <linux-arch@...r.kernel.org>,
	<linux-cxl@...r.kernel.org>, <linux-fsdevel@...r.kernel.org>,
	<linux-mm@...ck.org>, <linux-xfs@...r.kernel.org>,
	<dm-devel@...ts.linux.dev>, <nvdimm@...ts.linux.dev>,
	<linux-s390@...r.kernel.org>, Alasdair Kergon <agk@...hat.com>, Mike Snitzer
	<snitzer@...nel.org>, Mikulas Patocka <mpatocka@...hat.com>
Subject: Re: [PATCH v5 5/8] virtio: Treat alloc_dax() -EOPNOTSUPP failure as
 non-fatal

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.