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

[Ard Biesheuvel]

On Tue, 14 Mar 2023 at 23:25, Ard Biesheuvel <ardb at kernel.org> wrote:
>
> 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?

Actually, I ended up going a bit further than this, and reimplemented
vfp_support_entry() in C entirely. I will add that as a patch to the
next revision, but that one should not be backported.