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

[Ard Biesheuvel]

On Tue, 14 Mar 2023 at 22:46, Peter Zijlstra <peterz at infradead.org> wrote:
>
> 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);
> +}

That would be much better, yes.

These fixes ultimately need to be backported to v6.2 so I'm not sure
if we want to include this now, or keep it as a cleanup for the next
cycle?