Re: [PATCH] crypto: x86/aes-gcm: Disable FPU around skcipher_walk_done().

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

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