Message-ID: <CAMj1kXHB5-ZFbi5TfuU_pfNJRdxH5-ZUY+k4azpvYgv1Py_Ocw@mail.gmail.com>
Date: Thu, 3 Apr 2025 10:03:02 +0300
From: Ard Biesheuvel <ardb@...nel.org>
To: Eric Biggers <ebiggers@...nel.org>
Cc: Herbert Xu <herbert@...dor.apana.org.au>, linux-crypto@...r.kernel.org, 
	linux-kernel@...r.kernel.org, x86@...nel.org, 
	"Jason A. Donenfeld" <Jason@...c4.com>, Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: Banning crypto in hardirq context (was: [PATCH v2 0/9] crypto:
 x86 - stop using the SIMD helper)

On Thu, 3 Apr 2025 at 06:59, Eric Biggers <ebiggers@...nel.org> wrote:
>
> On Thu, Apr 03, 2025 at 11:42:34AM +0800, Herbert Xu wrote:
> > On Wed, Apr 02, 2025 at 08:20:08PM -0700, Eric Biggers wrote:
> > >
> > > Also, riscv has scalar AES instructions.  (They aren't used by the kernel yet,
> > > but they could be.  The CRC code already uses scalar carryless multiplication.)
> >
> > It still doesn't mean that it's a good idea to use AES in a
> > hard IRQ handler, especially if the code is meant to be portable.
> >
> > > Also, as I said already, x86 does support SIMD instructions in hardirq context
> > > in some cases.  Whether anyone actually uses that, I don't know, but it is
> > > explicitly supported.  Check out irq_fpu_usable().
> >
> > This is more of an accident than some deliberate strategy of
> > supporting FPU usage in hard IRQs.  This test was initially
> > added for aesni:
> >
> > commit 54b6a1bd5364aca95cd6ffae00f2b64c6511122c
> > Author: Ying Huang <huang.ying.caritas@...il.com>
> > Date:   Sun Jan 18 16:28:34 2009 +1100
> >
> >     crypto: aes-ni - Add support to Intel AES-NI instructions for x86_64 platform
> >
> > It was then improved by:
> >
> > Author: Linus Torvalds <torvalds@...ux-foundation.org>
> > Date:   Mon Feb 13 13:56:14 2012 -0800
> >
> >     i387: make irq_fpu_usable() tests more robust
> >
> >     Some code - especially the crypto layer - wants to use the x86
> >     FP/MMX/AVX register set in what may be interrupt (typically softirq)
> >     context.
> >
> > At no point was there any intention of using this in a hardirq
> > context.
> >
> > Until such a time when you have a valid application for using
> > lib/crypto code in a hardirq context, I don't think we should
> > be supporting that at the expense of real users who are in
> > process/softirq context only.
>
> Whatever.  We agree that "crypto in hardirq" is not a good idea in general.  I'm
> just pointing out that there are certain cases, like SipHash used in a hash
> table, where it easily could happen and would be fine.  And all the shash and
> crypto library functions currently work in any context, unlike e.g. skcipher and
> aead which do not.  You seem to be trying to claim that it was never supported,
> but that is incorrect.  Making it unsupported would be a change that needs to be
> properly documented (the functions would no longer be simply "Any context")
> *and* have proper debug assertions added to enforce it and prevent usage errors.
> But in a lot of cases there is also no reason to even add that restriction.  I'm
> not sure why you're so eager to make the library functions harder to use.
>

Agree with Eric.

There may be cases where some error condition (machine check etc) is
hit while running in hard IRQ context or with IRQs disabled, and the
code that produces the diagnostic, writes to pstore, generates the QR
code for  etc etc may actually be where the library calls to crc32 etc
originate from. So pedantically disallowing that rather than falling
back to a non-SIMD code path make things worse, because now, the
original diagnostic may get lost while the only information left to
debug the issue is an OOPS complaining about a library call in hard
IRQ context.

So while I agree that knowingly invoking library interfaces with IRQs
disabled should be avoided, that is just a variation on the general
adage that IRQs should only be disabled when absolutely necessary. But
that necessity may derive from a condition that exists one or several
layers up.

Powered by blists - more mailing lists