[PATCH v2 3/3] ARM: vfp: Fix broken softirq handling with instrumentation enabled

[Peter Zijlstra]

On Tue, Mar 14, 2023 at 09:46:21PM +0100, Linus Walleij wrote:
> Hi Ard,
> 
> On Tue, Mar 14, 2023 at 8:28 PM Ard Biesheuvel <ardb at kernel.org> wrote:
> 
> > Commit 62b95a7b44d1 ("ARM: 9282/1: vfp: Manipulate task VFP state with
> > softirqs disabled") replaced the en/disable preemption calls inside the
> > VFP state handling code with en/disabling of soft IRQs, which is
> > necessary to allow kernel use of the VFP/SIMD unit when handling a soft
> > IRQ.
> >
> > Unfortunately, when lockdep is enabled (or other instrumentation that
> > enables TRACE_IRQFLAGS), the disable path implemented in asm fails to
> > perform the lockdep and RCU related bookkeeping, resulting in spurious
> > warnings and other badness.
> 
> Not good.
> 
> > Set let's rework the VFP entry code a little bit so we can make the
> > local_bh_disable() call from C, with all the instrumentations that
> > happen to have been configured. Calling local_bh_enable() can be done
> > from asm, as it is always a callable function.
> 
> Here it says local_bh_enable() is called
> 
> (...)
> 
> > +local_bh_enable_and_ret:
> > +       adr     r0, .
> > +       mov     r1, #SOFTIRQ_DISABLE_OFFSET
> > +       b       __local_bh_enable_ip    @ tail call
> 
> Please add a comment here that this is an open coded version of
> local_bh_enable() from the <linux/bottom_half.h> header file
> and we hope that inline code will not change.
> 
> I.e this:
> 
> static inline void local_bh_enable(void)
> {
>         __local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
> }
> 
> I almost want to make a warning comment into <linux/bottom_half.h>
> that the same function exists in an open coded version in arch/arm/vfp/vfphw.S
> so this static inline cannot be refactored without consequences.

So we could do something like the below to make local_bh_enable() both
an inline and an actual symbol for those who need to call it from asm.

It seems to compile on both GCC-12 and Clang-15.

diff --git a/include/linux/bottom_half.h b/include/linux/bottom_half.h
index fc53e0ad56d9..85fcdf647f9f 100644
--- a/include/linux/bottom_half.h
+++ b/include/linux/bottom_half.h
@@ -28,7 +28,7 @@ static inline void local_bh_enable_ip(unsigned long ip)
 	__local_bh_enable_ip(ip, SOFTIRQ_DISABLE_OFFSET);
 }
 
-static inline void local_bh_enable(void)
+extern inline void local_bh_enable(void)
 {
 	__local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
 }
diff --git a/kernel/softirq.c b/kernel/softirq.c
index c8a6913c067d..91d8677f890a 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -1010,3 +1010,8 @@ unsigned int __weak arch_dynirq_lower_bound(unsigned int from)
 {
 	return from;
 }
+
+void local_bh_enable(void)
+{
+	__local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
+}