* [PATCH] crypto: x86/aes-gcm: Disable FPU around skcipher_walk_done().
@ 2024-08-02 10:23 Sebastian Andrzej Siewior
2024-08-02 16:28 ` Eric Biggers
2024-08-03 0:37 ` Herbert Xu
0 siblings, 2 replies; 12+ messages in thread
From: Sebastian Andrzej Siewior @ 2024-08-02 10:23 UTC (permalink / raw)
To: linux-crypto; +Cc: David S. Miller, Eric Biggers, Herbert Xu, Thomas Gleixner
kernel_fpu_begin() disables preemption. gcm_crypt() has a
skcipher_walk_done() invocation within a preempt disabled section.
skcipher_walk_done() can invoke kfree() which requires sleeping locks on
PREEMPT_RT and must not be invoked with disabled preemption.
Keep FPU access enabled while skcipher_walk_done() is invoked.
Fixes: b06affb1cb580 ("crypto: x86/aes-gcm - add VAES and AVX512 / AVX10 optimized AES-GCM")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
arch/x86/crypto/aesni-intel_glue.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
index cd37de5ec4046..be92e4c3f9c7f 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -1403,7 +1403,9 @@ gcm_crypt(struct aead_request *req, int flags)
aes_gcm_update(key, le_ctr, ghash_acc,
walk.src.virt.addr, walk.dst.virt.addr,
nbytes, flags);
+ kernel_fpu_end();
err = skcipher_walk_done(&walk, 0);
+ kernel_fpu_begin();
/*
* The low word of the counter isn't used by the
* finalize, so there's no need to increment it here.
--
2.45.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] crypto: x86/aes-gcm: Disable FPU around skcipher_walk_done().
2024-08-02 10:23 [PATCH] crypto: x86/aes-gcm: Disable FPU around skcipher_walk_done() Sebastian Andrzej Siewior
@ 2024-08-02 16:28 ` Eric Biggers
2024-08-02 16:49 ` Eric Biggers
` (2 more replies)
2024-08-03 0:37 ` Herbert Xu
1 sibling, 3 replies; 12+ messages in thread
From: Eric Biggers @ 2024-08-02 16:28 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: linux-crypto, David S. Miller, Herbert Xu, Thomas Gleixner
Hi Sebastian,
On Fri, Aug 02, 2024 at 12:23:33PM +0200, Sebastian Andrzej Siewior wrote:
> kernel_fpu_begin() disables preemption. gcm_crypt() has a
> skcipher_walk_done() invocation within a preempt disabled section.
> skcipher_walk_done() can invoke kfree() which requires sleeping locks on
> PREEMPT_RT and must not be invoked with disabled preemption.
>
> Keep FPU access enabled while skcipher_walk_done() is invoked.
>
> Fixes: b06affb1cb580 ("crypto: x86/aes-gcm - add VAES and AVX512 / AVX10 optimized AES-GCM")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> arch/x86/crypto/aesni-intel_glue.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
> index cd37de5ec4046..be92e4c3f9c7f 100644
> --- a/arch/x86/crypto/aesni-intel_glue.c
> +++ b/arch/x86/crypto/aesni-intel_glue.c
> @@ -1403,7 +1403,9 @@ gcm_crypt(struct aead_request *req, int flags)
> aes_gcm_update(key, le_ctr, ghash_acc,
> walk.src.virt.addr, walk.dst.virt.addr,
> nbytes, flags);
> + kernel_fpu_end();
> err = skcipher_walk_done(&walk, 0);
> + kernel_fpu_begin();
> /*
> * The low word of the counter isn't used by the
> * finalize, so there's no need to increment it here.
Can you make this conditional on CONFIG_PREEMPT_RT so that it doesn't hurt
performance for everyone else?
Note that kfree() lacks a might_sleep(), and its kerneldoc does not say that it
can sleep. Have you checked for other instances of this same problem? It seems
it would be quite common kernel-wide. Is it really necessary that kfree() takes
a sleepable lock on PREEMPT_RT?
- Eric
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] crypto: x86/aes-gcm: Disable FPU around skcipher_walk_done().
2024-08-02 16:28 ` Eric Biggers
@ 2024-08-02 16:49 ` Eric Biggers
2024-08-05 9:02 ` Herbert Xu
2024-08-03 0:34 ` Herbert Xu
2024-08-05 8:41 ` Sebastian Andrzej Siewior
2 siblings, 1 reply; 12+ messages in thread
From: Eric Biggers @ 2024-08-02 16:49 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: linux-crypto, David S. Miller, Herbert Xu, Thomas Gleixner
On Fri, Aug 02, 2024 at 09:28:32AM -0700, Eric Biggers wrote:
> Hi Sebastian,
>
> On Fri, Aug 02, 2024 at 12:23:33PM +0200, Sebastian Andrzej Siewior wrote:
> > kernel_fpu_begin() disables preemption. gcm_crypt() has a
> > skcipher_walk_done() invocation within a preempt disabled section.
> > skcipher_walk_done() can invoke kfree() which requires sleeping locks on
> > PREEMPT_RT and must not be invoked with disabled preemption.
> >
> > Keep FPU access enabled while skcipher_walk_done() is invoked.
> >
> > Fixes: b06affb1cb580 ("crypto: x86/aes-gcm - add VAES and AVX512 / AVX10 optimized AES-GCM")
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> > arch/x86/crypto/aesni-intel_glue.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
> > index cd37de5ec4046..be92e4c3f9c7f 100644
> > --- a/arch/x86/crypto/aesni-intel_glue.c
> > +++ b/arch/x86/crypto/aesni-intel_glue.c
> > @@ -1403,7 +1403,9 @@ gcm_crypt(struct aead_request *req, int flags)
> > aes_gcm_update(key, le_ctr, ghash_acc,
> > walk.src.virt.addr, walk.dst.virt.addr,
> > nbytes, flags);
> > + kernel_fpu_end();
> > err = skcipher_walk_done(&walk, 0);
> > + kernel_fpu_begin();
> > /*
> > * The low word of the counter isn't used by the
> > * finalize, so there's no need to increment it here.
>
> Can you make this conditional on CONFIG_PREEMPT_RT so that it doesn't hurt
> performance for everyone else?
>
> Note that kfree() lacks a might_sleep(), and its kerneldoc does not say that it
> can sleep. Have you checked for other instances of this same problem? It seems
> it would be quite common kernel-wide. Is it really necessary that kfree() takes
> a sleepable lock on PREEMPT_RT?
>
This would work too, I think:
diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
index cd37de5ec4046..2d6bcf7fc7c51 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -1401,11 +1401,12 @@ gcm_crypt(struct aead_request *req, int flags)
} else {
/* Last segment: process all remaining data. */
aes_gcm_update(key, le_ctr, ghash_acc,
walk.src.virt.addr, walk.dst.virt.addr,
nbytes, flags);
- err = skcipher_walk_done(&walk, 0);
+ err = 0;
+ break;
/*
* The low word of the counter isn't used by the
* finalize, so there's no need to increment it here.
*/
}
@@ -1439,10 +1440,12 @@ gcm_crypt(struct aead_request *req, int flags)
datalen, tag, taglen, flags))
err = -EBADMSG;
}
out:
kernel_fpu_end();
+ if (nbytes)
+ skcipher_walk_done(&walk, 0);
return err;
}
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] crypto: x86/aes-gcm: Disable FPU around skcipher_walk_done().
2024-08-02 16:28 ` Eric Biggers
2024-08-02 16:49 ` Eric Biggers
@ 2024-08-03 0:34 ` Herbert Xu
2024-08-05 8:21 ` Sebastian Andrzej Siewior
2024-08-05 8:41 ` Sebastian Andrzej Siewior
2 siblings, 1 reply; 12+ messages in thread
From: Herbert Xu @ 2024-08-03 0:34 UTC (permalink / raw)
To: Eric Biggers
Cc: Sebastian Andrzej Siewior, linux-crypto, David S. Miller,
Thomas Gleixner
On Fri, Aug 02, 2024 at 09:28:32AM -0700, Eric Biggers wrote:
>
> Note that kfree() lacks a might_sleep(), and its kerneldoc does not say that it
> can sleep. Have you checked for other instances of this same problem? It seems
> it would be quite common kernel-wide. Is it really necessary that kfree() takes
> a sleepable lock on PREEMPT_RT?
Agreed. kfree() gets called in all sorts of places under softirq
context in the network stack which would presumably have the same
issue.
Please give a bit of context of when kfree started doing this.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] crypto: x86/aes-gcm: Disable FPU around skcipher_walk_done().
2024-08-02 10:23 [PATCH] crypto: x86/aes-gcm: Disable FPU around skcipher_walk_done() Sebastian Andrzej Siewior
2024-08-02 16:28 ` Eric Biggers
@ 2024-08-03 0:37 ` Herbert Xu
2024-08-05 9:34 ` Sebastian Andrzej Siewior
1 sibling, 1 reply; 12+ messages in thread
From: Herbert Xu @ 2024-08-03 0:37 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: linux-crypto, David S. Miller, Eric Biggers, Thomas Gleixner
On Fri, Aug 02, 2024 at 12:23:33PM +0200, Sebastian Andrzej Siewior wrote:
> kernel_fpu_begin() disables preemption. gcm_crypt() has a
> skcipher_walk_done() invocation within a preempt disabled section.
> skcipher_walk_done() can invoke kfree() which requires sleeping locks on
> PREEMPT_RT and must not be invoked with disabled preemption.
>
> Keep FPU access enabled while skcipher_walk_done() is invoked.
>
> Fixes: b06affb1cb580 ("crypto: x86/aes-gcm - add VAES and AVX512 / AVX10 optimized AES-GCM")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> arch/x86/crypto/aesni-intel_glue.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
> index cd37de5ec4046..be92e4c3f9c7f 100644
> --- a/arch/x86/crypto/aesni-intel_glue.c
> +++ b/arch/x86/crypto/aesni-intel_glue.c
> @@ -1403,7 +1403,9 @@ gcm_crypt(struct aead_request *req, int flags)
> aes_gcm_update(key, le_ctr, ghash_acc,
> walk.src.virt.addr, walk.dst.virt.addr,
> nbytes, flags);
> + kernel_fpu_end();
> err = skcipher_walk_done(&walk, 0);
> + kernel_fpu_begin();
What if the user already did a preempt_disable()? This would still
be buggy, right?
The Crypto API allows this to be called with preemption disabled.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] crypto: x86/aes-gcm: Disable FPU around skcipher_walk_done().
2024-08-03 0:34 ` Herbert Xu
@ 2024-08-05 8:21 ` Sebastian Andrzej Siewior
0 siblings, 0 replies; 12+ messages in thread
From: Sebastian Andrzej Siewior @ 2024-08-05 8:21 UTC (permalink / raw)
To: Herbert Xu; +Cc: Eric Biggers, linux-crypto, David S. Miller, Thomas Gleixner
On 2024-08-03 08:34:05 [+0800], Herbert Xu wrote:
> On Fri, Aug 02, 2024 at 09:28:32AM -0700, Eric Biggers wrote:
> >
> > Note that kfree() lacks a might_sleep(), and its kerneldoc does not say that it
> > can sleep. Have you checked for other instances of this same problem? It seems
> > it would be quite common kernel-wide. Is it really necessary that kfree() takes
> > a sleepable lock on PREEMPT_RT?
>
> Agreed. kfree() gets called in all sorts of places under softirq
> context in the network stack which would presumably have the same
> issue.
>
> Please give a bit of context of when kfree started doing this.
I added "under PREEMPT_RT" in the patch description. The softirq under
PREEMPT_RT does not disable preemption so it is fully preemptible.
The other accelerated versions drop exclusive FPU access/ enable
preemption during these operations even on ARM/ ARM64.
> Cheers,
Sebastian
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] crypto: x86/aes-gcm: Disable FPU around skcipher_walk_done().
2024-08-02 16:28 ` Eric Biggers
2024-08-02 16:49 ` Eric Biggers
2024-08-03 0:34 ` Herbert Xu
@ 2024-08-05 8:41 ` Sebastian Andrzej Siewior
2024-08-05 17:38 ` Eric Biggers
2 siblings, 1 reply; 12+ messages in thread
From: Sebastian Andrzej Siewior @ 2024-08-05 8:41 UTC (permalink / raw)
To: Eric Biggers; +Cc: linux-crypto, David S. Miller, Herbert Xu, Thomas Gleixner
On 2024-08-02 09:28:32 [-0700], Eric Biggers wrote:
> Hi Sebastian,
Hi Eric,
> > diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
> > index cd37de5ec4046..be92e4c3f9c7f 100644
> > --- a/arch/x86/crypto/aesni-intel_glue.c
> > +++ b/arch/x86/crypto/aesni-intel_glue.c
> > @@ -1403,7 +1403,9 @@ gcm_crypt(struct aead_request *req, int flags)
> > aes_gcm_update(key, le_ctr, ghash_acc,
> > walk.src.virt.addr, walk.dst.virt.addr,
> > nbytes, flags);
> > + kernel_fpu_end();
> > err = skcipher_walk_done(&walk, 0);
> > + kernel_fpu_begin();
> > /*
> > * The low word of the counter isn't used by the
> > * finalize, so there's no need to increment it here.
>
> Can you make this conditional on CONFIG_PREEMPT_RT so that it doesn't hurt
> performance for everyone else?
Every other instance in this file had a kernel_fpu_end/ begin() before
skcipher_walk_done() so I though was just missed by chance.
> Note that kfree() lacks a might_sleep(), and its kerneldoc does not say that it
> can sleep. Have you checked for other instances of this same problem? It seems
> it would be quite common kernel-wide.
kfree() can't have a might_sleep() because it does not qualify for this
since you can use it in softirq context for instance with an acquired
spinlockt_t on !RT which would trigger it.
On PREEMPT_RT interrupts are threaded, softirq is preemptible,
spintlock_t is a sleeping lock so all these things where a kfree()
would have been invoked in preempt-disable context on !PREEMPT_RT is
actually preemptible on PREEMPT_RT.
This is of course not true in cases where preemption is explicitly
disabled like in this case.
> Is it really necessary that kfree() takes
> a sleepable lock on PREEMPT_RT?
Yes. The locking in SLUB and page allocator is spinlock_t.
> - Eric
Sebastian
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] crypto: x86/aes-gcm: Disable FPU around skcipher_walk_done().
2024-08-02 16:49 ` Eric Biggers
@ 2024-08-05 9:02 ` Herbert Xu
2024-08-05 9:56 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 12+ messages in thread
From: Herbert Xu @ 2024-08-05 9:02 UTC (permalink / raw)
To: Eric Biggers
Cc: Sebastian Andrzej Siewior, linux-crypto, David S. Miller,
Thomas Gleixner
On Fri, Aug 02, 2024 at 09:49:04AM -0700, Eric Biggers wrote:
>
> This would work too, I think:
Yes, and we can go a bit further like this:
diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
index cd37de5ec404..149bc6beae51 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -1381,8 +1381,9 @@ gcm_crypt(struct aead_request *req, int flags)
gcm_process_assoc(key, ghash_acc, req->src, assoclen, flags);
/* En/decrypt the data and pass the ciphertext through GHASH. */
- while ((nbytes = walk.nbytes) != 0) {
- if (unlikely(nbytes < walk.total)) {
+ nbytes = walk.nbytes;
+ if (nbytes) {
+ while (unlikely(nbytes < walk.total)) {
/*
* Non-last segment. In this case, the assembly
* function requires that the length be a multiple of 16
@@ -1397,21 +1398,24 @@ gcm_crypt(struct aead_request *req, int flags)
le_ctr[0] += nbytes / AES_BLOCK_SIZE;
kernel_fpu_end();
err = skcipher_walk_done(&walk, walk.nbytes - nbytes);
+ if (err)
+ return err;
+ nbytes = walk.nbytes;
kernel_fpu_begin();
- } else {
- /* Last segment: process all remaining data. */
- aes_gcm_update(key, le_ctr, ghash_acc,
- walk.src.virt.addr, walk.dst.virt.addr,
- nbytes, flags);
- err = skcipher_walk_done(&walk, 0);
- /*
- * The low word of the counter isn't used by the
- * finalize, so there's no need to increment it here.
- */
}
+
+ /* Last segment: process all remaining data. */
+ aes_gcm_update(key, le_ctr, ghash_acc,
+ walk.src.virt.addr, walk.dst.virt.addr,
+ nbytes, flags);
+ /*
+ * The low word of the counter isn't used by the
+ * finalize, so there's no need to increment it here.
+ */
+ } else if (err) {
+ kernel_fpu_end();
+ return err;
}
- if (err)
- goto out;
/* Finalize */
taglen = crypto_aead_authsize(tfm);
@@ -1439,9 +1443,8 @@ gcm_crypt(struct aead_request *req, int flags)
datalen, tag, taglen, flags))
err = -EBADMSG;
}
-out:
kernel_fpu_end();
- return err;
+ return skcipher_walk_done(&walk, 0);
}
#define DEFINE_GCM_ALGS(suffix, flags, generic_driver_name, rfc_driver_name, \
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] crypto: x86/aes-gcm: Disable FPU around skcipher_walk_done().
2024-08-03 0:37 ` Herbert Xu
@ 2024-08-05 9:34 ` Sebastian Andrzej Siewior
0 siblings, 0 replies; 12+ messages in thread
From: Sebastian Andrzej Siewior @ 2024-08-05 9:34 UTC (permalink / raw)
To: Herbert Xu; +Cc: linux-crypto, David S. Miller, Eric Biggers, Thomas Gleixner
On 2024-08-03 08:37:05 [+0800], Herbert Xu wrote:
> On Fri, Aug 02, 2024 at 12:23:33PM +0200, Sebastian Andrzej Siewior wrote:
> > kernel_fpu_begin() disables preemption. gcm_crypt() has a
> > skcipher_walk_done() invocation within a preempt disabled section.
> > skcipher_walk_done() can invoke kfree() which requires sleeping locks on
> > PREEMPT_RT and must not be invoked with disabled preemption.
> >
> > Keep FPU access enabled while skcipher_walk_done() is invoked.
> >
> > Fixes: b06affb1cb580 ("crypto: x86/aes-gcm - add VAES and AVX512 / AVX10 optimized AES-GCM")
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> > arch/x86/crypto/aesni-intel_glue.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
> > index cd37de5ec4046..be92e4c3f9c7f 100644
> > --- a/arch/x86/crypto/aesni-intel_glue.c
> > +++ b/arch/x86/crypto/aesni-intel_glue.c
> > @@ -1403,7 +1403,9 @@ gcm_crypt(struct aead_request *req, int flags)
> > aes_gcm_update(key, le_ctr, ghash_acc,
> > walk.src.virt.addr, walk.dst.virt.addr,
> > nbytes, flags);
> > + kernel_fpu_end();
> > err = skcipher_walk_done(&walk, 0);
> > + kernel_fpu_begin();
>
> What if the user already did a preempt_disable()? This would still
> be buggy, right?
Yes if it has been done explicitly by preempt_disable(). And I am
looking into explicit case of disabling preemption and trying to get rid
of it if I stumble upon one. This one just popped up on one of my boxes.
> The Crypto API allows this to be called with preemption disabled.
>
> Cheers,
Sebastian
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] crypto: x86/aes-gcm: Disable FPU around skcipher_walk_done().
2024-08-05 9:02 ` Herbert Xu
@ 2024-08-05 9:56 ` Sebastian Andrzej Siewior
0 siblings, 0 replies; 12+ messages in thread
From: Sebastian Andrzej Siewior @ 2024-08-05 9:56 UTC (permalink / raw)
To: Herbert Xu; +Cc: Eric Biggers, linux-crypto, David S. Miller, Thomas Gleixner
On 2024-08-05 17:02:35 [+0800], Herbert Xu wrote:
> On Fri, Aug 02, 2024 at 09:49:04AM -0700, Eric Biggers wrote:
> >
> > This would work too, I think:
>
> Yes, and we can go a bit further like this:
Yes,
Tested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Sebastian
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] crypto: x86/aes-gcm: Disable FPU around skcipher_walk_done().
2024-08-05 8:41 ` Sebastian Andrzej Siewior
@ 2024-08-05 17:38 ` Eric Biggers
2024-08-06 7:46 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 12+ messages in thread
From: Eric Biggers @ 2024-08-05 17:38 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: linux-crypto, David S. Miller, Herbert Xu, Thomas Gleixner
On Mon, Aug 05, 2024 at 10:41:21AM +0200, Sebastian Andrzej Siewior wrote:
> On 2024-08-02 09:28:32 [-0700], Eric Biggers wrote:
> > Hi Sebastian,
> Hi Eric,
>
> > > diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
> > > index cd37de5ec4046..be92e4c3f9c7f 100644
> > > --- a/arch/x86/crypto/aesni-intel_glue.c
> > > +++ b/arch/x86/crypto/aesni-intel_glue.c
> > > @@ -1403,7 +1403,9 @@ gcm_crypt(struct aead_request *req, int flags)
> > > aes_gcm_update(key, le_ctr, ghash_acc,
> > > walk.src.virt.addr, walk.dst.virt.addr,
> > > nbytes, flags);
> > > + kernel_fpu_end();
> > > err = skcipher_walk_done(&walk, 0);
> > > + kernel_fpu_begin();
> > > /*
> > > * The low word of the counter isn't used by the
> > > * finalize, so there's no need to increment it here.
> >
> > Can you make this conditional on CONFIG_PREEMPT_RT so that it doesn't hurt
> > performance for everyone else?
>
> Every other instance in this file had a kernel_fpu_end/ begin() before
> skcipher_walk_done() so I though was just missed by chance.
No, it was intentional. See the comment above the first kernel_fpu_begin() in
gcm_crypt():
/*
* Since the AES-GCM assembly code requires that at least three assembly
* functions be called to process any message (this is needed to support
* incremental updates cleanly), to reduce overhead we try to do all
* three calls in the same kernel FPU section if possible. We close the
* section and start a new one if there are multiple data segments or if
* rescheduling is needed while processing the associated data.
*/
kernel_fpu_begin();
> > Note that kfree() lacks a might_sleep(), and its kerneldoc does not say that it
> > can sleep. Have you checked for other instances of this same problem? It seems
> > it would be quite common kernel-wide.
>
> kfree() can't have a might_sleep() because it does not qualify for this
> since you can use it in softirq context for instance with an acquired
> spinlockt_t on !RT which would trigger it.
> On PREEMPT_RT interrupts are threaded, softirq is preemptible,
> spintlock_t is a sleeping lock so all these things where a kfree()
> would have been invoked in preempt-disable context on !PREEMPT_RT is
> actually preemptible on PREEMPT_RT.
> This is of course not true in cases where preemption is explicitly
> disabled like in this case.
WARN_ON(!preemptible()) then?
If I add that to kfree(), it triggers from lots of other places. Are those
problems on PREEMPT_RT too?
What I am trying to get at is what debugging options do I need to detect issues
like this. Is there really no option other than actually running a PREEMPT_RT
kernel?
I had tested this code with lots of debug options pre-merge and nothing came up.
If there was something in CONFIG_SLUB_DEBUG, for example, I would have seen
that, and you would never have had to deal with this issue at all as it would
never have been introduced.
- Eric
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] crypto: x86/aes-gcm: Disable FPU around skcipher_walk_done().
2024-08-05 17:38 ` Eric Biggers
@ 2024-08-06 7:46 ` Sebastian Andrzej Siewior
0 siblings, 0 replies; 12+ messages in thread
From: Sebastian Andrzej Siewior @ 2024-08-06 7:46 UTC (permalink / raw)
To: Eric Biggers; +Cc: linux-crypto, David S. Miller, Herbert Xu, Thomas Gleixner
On 2024-08-05 10:38:26 [-0700], Eric Biggers wrote:
> No, it was intentional. See the comment above the first kernel_fpu_begin() in
> gcm_crypt():
>
> /*
> * Since the AES-GCM assembly code requires that at least three assembly
> * functions be called to process any message (this is needed to support
> * incremental updates cleanly), to reduce overhead we try to do all
> * three calls in the same kernel FPU section if possible. We close the
> * section and start a new one if there are multiple data segments or if
> * rescheduling is needed while processing the associated data.
> */
> kernel_fpu_begin();
Okay.
> > > Note that kfree() lacks a might_sleep(), and its kerneldoc does not say that it
> > > can sleep. Have you checked for other instances of this same problem? It seems
> > > it would be quite common kernel-wide.
> >
> > kfree() can't have a might_sleep() because it does not qualify for this
> > since you can use it in softirq context for instance with an acquired
> > spinlockt_t on !RT which would trigger it.
> > On PREEMPT_RT interrupts are threaded, softirq is preemptible,
> > spintlock_t is a sleeping lock so all these things where a kfree()
> > would have been invoked in preempt-disable context on !PREEMPT_RT is
> > actually preemptible on PREEMPT_RT.
> > This is of course not true in cases where preemption is explicitly
> > disabled like in this case.
>
> WARN_ON(!preemptible()) then?
>
> If I add that to kfree(), it triggers from lots of other places. Are those
> problems on PREEMPT_RT too?
Nope, shouldn't. On PREEMPT_RT you can only invoke kfree() or kmalloc()
from preemptible context. This is not a problem since interrupts are
threaded, softirqs are preemptbile,…
> What I am trying to get at is what debugging options do I need to detect issues
> like this. Is there really no option other than actually running a PREEMPT_RT
> kernel?
There is PROVE_RAW_LOCK_NESTING but this only catches something like
raw_spinlock_t -> spinlock_t or using a spinlock_t in an interrupt
handler that won't be threaded. It won't find anything where you disable
interrupts or preemption on purpose.
> I had tested this code with lots of debug options pre-merge and nothing came up.
>
> If there was something in CONFIG_SLUB_DEBUG, for example, I would have seen
> that, and you would never have had to deal with this issue at all as it would
> never have been introduced.
Don't worry. I didn't think for a second that there was lack of testing
on your side.
> - Eric
Sebastian
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-08-06 7:46 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-02 10:23 [PATCH] crypto: x86/aes-gcm: Disable FPU around skcipher_walk_done() Sebastian Andrzej Siewior
2024-08-02 16:28 ` Eric Biggers
2024-08-02 16:49 ` Eric Biggers
2024-08-05 9:02 ` Herbert Xu
2024-08-05 9:56 ` Sebastian Andrzej Siewior
2024-08-03 0:34 ` Herbert Xu
2024-08-05 8:21 ` Sebastian Andrzej Siewior
2024-08-05 8:41 ` Sebastian Andrzej Siewior
2024-08-05 17:38 ` Eric Biggers
2024-08-06 7:46 ` Sebastian Andrzej Siewior
2024-08-03 0:37 ` Herbert Xu
2024-08-05 9:34 ` Sebastian Andrzej Siewior
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox