[PATCH v2] ARM: fix Thumb2 regression with Spectre BHB

[PATCH v2] ARM: fix Thumb2 regression with Spectre BHB

[Russell King (Oracle)]

When building for Thumb2, the vectors make use of a local label. Sadly,
the Spectre BHB code also uses a local label with the same number which
results in the Thumb2 reference pointing at the wrong place. Fix this
by changing the number used for the Spectre BHB local label.

Fixes: b9baf5c8c5c3 ("ARM: Spectre-BHB workaround")
Tested-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 arch/arm/kernel/entry-armv.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index faa0ca2ba576..63a9d59478fa 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -962,9 +962,9 @@ ENDPROC(vector_\name)
 
 	@ bhb workaround
 	mov	r0, #8
-1:	b	. + 4
+3:	b	. + 4
 	subs	r0, r0, #1
-	bne	1b
+	bne	3b
 	dsb
 	isb
 	b	2b
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] ARM: fix Thumb2 regression with Spectre BHB

[Linus Torvalds]

On Fri, Mar 11, 2022 at 9:13 AM Russell King (Oracle)
<rmk+kernel@armlinux.org.uk> wrote:
>
> When building for Thumb2, the vectors make use of a local label. Sadly,
> the Spectre BHB code also uses a local label with the same number which
> results in the Thumb2 reference pointing at the wrong place. Fix this
> by changing the number used for the Spectre BHB local label.

Ugh. Those local names are nasty.

And even with this patch - which I'm about to apply right now - I
think the numbering is horrendous. It seems entirely random with "2"
being used first in that vector_stub macro, and now 1 and 3 in the
middle.

I assume it's entirely historical, and the numbering reflects the
order the labels were added, rather than anything logical.

Yes, local labels are really convenient in assembler macros, but I can
also heartily suggest using numbers that stand out more and are more
unique. Certainly not 1/2/3 in an odd and illogical order.

             Linus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] ARM: fix Thumb2 regression with Spectre BHB

[Linus Torvalds]

On Fri, Mar 11, 2022 at 11:39 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> And even with this patch - which I'm about to apply right now - I
> think the numbering is horrendous [..]

Side note: having applied it, I notice that the line numbers don't
match even remotely.  In your patch, it's

   @@ -962,9 +962,9 @@ ENDPROC(vector_\name)

but in my tree it ends up being

  @@ -1040,9 +1040,9 @@ vector_bhb_loop8_\name:

Yes, git finds the right place thanks to the context (and I verified
that there is no other possible place), but I wonder what the base was
here. Not my current tree, for sure.

                      Linus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] ARM: fix Thumb2 regression with Spectre BHB

[Russell King (Oracle)]

On Fri, Mar 11, 2022 at 11:39:35AM -0800, Linus Torvalds wrote:
> On Fri, Mar 11, 2022 at 9:13 AM Russell King (Oracle)
> <rmk+kernel@armlinux.org.uk> wrote:
> >
> > When building for Thumb2, the vectors make use of a local label. Sadly,
> > the Spectre BHB code also uses a local label with the same number which
> > results in the Thumb2 reference pointing at the wrong place. Fix this
> > by changing the number used for the Spectre BHB local label.
> 
> Ugh. Those local names are nasty.
> 
> And even with this patch - which I'm about to apply right now - I
> think the numbering is horrendous. It seems entirely random with "2"
> being used first in that vector_stub macro, and now 1 and 3 in the
> middle.

I don't like them, but in a macro, I don't think there's much choice.
I don't think choosing some other starting number works either, because
then one ends up always starting at some other number and it's just the
same problem, just with a different starting number.

I guess one way around this would be to use a random local label
number each time, but I suspect there isn't enough range to ensure
they are unique using that method.

I've already mentioned to Nathan that I really wish gas etc would
allow local labels which are not purely numerical (so we can make
them more descriptive.) However, I can see why it isn't permitted,
because of the 'b' and 'f' suffix.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] ARM: fix Thumb2 regression with Spectre BHB

[Linus Torvalds]

On Fri, Mar 11, 2022 at 11:59 AM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> I don't like them, but in a macro, I don't think there's much choice.
> I don't think choosing some other starting number works either, because
> then one ends up always starting at some other number and it's just the
> same problem, just with a different starting number.

In some x86 code, people use labels like "99:" etc because they simply
stand out more visually.

Also, you can simply use *real* label names in macros, and just append
'\@' to them (which is basically a macro counter value).

IOW, I think that loop could have been written as

  bhbloop_\@:
        b       . + 4
        subs    r0, r0, #1
        bne     bhbloop_\@

and it should work, and also be more legible than using a random small integer.

> I've already mentioned to Nathan that I really wish gas etc would
> allow local labels which are not purely numerical (so we can make
> them more descriptive.) However, I can see why it isn't permitted,
> because of the 'b' and 'f' suffix.

Yeah, I don't think we can get away from numerical names for these
local (repeated) uses in inline asms at least.

But I do think that making them a bit more unique by using bigger
numbers can be at least a visual clue.

And in actual assembler macros, maybe that "use a named variable with
\@" approach would be something we can move towards?

              Linue

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel