diff mbox series

[v5,1/2] ARM: kprobes: fix UNPREDICTABLE warnings

Message ID 20210211025149.3544593-2-ndesaulniers@google.com (mailing list archive)
State New, archived
Headers show
Series ARM: kprobes: asm warning fixes and UAL conversion | expand

Commit Message

Nick Desaulniers Feb. 11, 2021, 2:51 a.m. UTC
GNU as warns twice for this file:
Warning: using r15 results in unpredictable behaviour

via the Arm ARM:
K1.1.1 Overview of the constraints on Armv7 UNPREDICTABLE behaviors

  The term UNPREDICTABLE describes a number of cases where the
  architecture has a feature that software must not use.

Link: https://github.com/ClangBuiltLinux/linux/issues/1271
Link: https://reviews.llvm.org/D95586
Reported-by: kernelci.org bot <bot@kernelci.org>
Suggested-by: Peter Smith <peter.smith@arm.com>
Suggested-by: Renato Golin <rengolin@systemcall.eu>
Suggested-by: David Spickett <david.spickett@linaro.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 arch/arm/probes/kprobes/test-arm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Ard Biesheuvel Feb. 11, 2021, 8:15 a.m. UTC | #1
On Thu, 11 Feb 2021 at 03:52, Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> GNU as warns twice for this file:
> Warning: using r15 results in unpredictable behaviour
>
> via the Arm ARM:
> K1.1.1 Overview of the constraints on Armv7 UNPREDICTABLE behaviors
>
>   The term UNPREDICTABLE describes a number of cases where the
>   architecture has a feature that software must not use.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1271
> Link: https://reviews.llvm.org/D95586
> Reported-by: kernelci.org bot <bot@kernelci.org>
> Suggested-by: Peter Smith <peter.smith@arm.com>
> Suggested-by: Renato Golin <rengolin@systemcall.eu>
> Suggested-by: David Spickett <david.spickett@linaro.org>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

Acked-by: Ard Biesheuvel <ardb@kernel.org>

But can we add a bit more context to the commit log, please? It is not
obvious to the casual reader why it is OK to emit UNPREDICTABLE
opcodes, i.e., that these are selftests that aim to ensure that kprobe
never attempts to replace the opcodes in question with a probe, but
that they are not actually executed, or expected to occur in real
code.

> ---
>  arch/arm/probes/kprobes/test-arm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/probes/kprobes/test-arm.c b/arch/arm/probes/kprobes/test-arm.c
> index 977369f1aa48..2543106a203e 100644
> --- a/arch/arm/probes/kprobes/test-arm.c
> +++ b/arch/arm/probes/kprobes/test-arm.c
> @@ -166,10 +166,10 @@ void kprobe_arm_test_cases(void)
>
>         /* Data-processing with PC as a target and status registers updated */
>         TEST_UNSUPPORTED("movs  pc, r1")
> -       TEST_UNSUPPORTED("movs  pc, r1, lsl r2")
> +       TEST_UNSUPPORTED(__inst_arm(0xe1b0f211) "       @movs   pc, r1, lsl r2")
>         TEST_UNSUPPORTED("movs  pc, #0x10000")
>         TEST_UNSUPPORTED("adds  pc, lr, r1")
> -       TEST_UNSUPPORTED("adds  pc, lr, r1, lsl r2")
> +       TEST_UNSUPPORTED(__inst_arm(0xe09ef211) "       @adds   pc, lr, r1, lsl r2")
>         TEST_UNSUPPORTED("adds  pc, lr, #4")
>
>         /* Data-processing with SP as target */
> --
> 2.30.0.478.g8a0d178c01-goog
>
Nick Desaulniers Feb. 11, 2021, 7:05 p.m. UTC | #2
On Thu, Feb 11, 2021 at 12:15 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 11 Feb 2021 at 03:52, Nick Desaulniers <ndesaulniers@google.com> wrote:
> >
> > GNU as warns twice for this file:
> > Warning: using r15 results in unpredictable behaviour
> >
> > via the Arm ARM:
> > K1.1.1 Overview of the constraints on Armv7 UNPREDICTABLE behaviors
> >
> >   The term UNPREDICTABLE describes a number of cases where the
> >   architecture has a feature that software must not use.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1271
> > Link: https://reviews.llvm.org/D95586
> > Reported-by: kernelci.org bot <bot@kernelci.org>
> > Suggested-by: Peter Smith <peter.smith@arm.com>
> > Suggested-by: Renato Golin <rengolin@systemcall.eu>
> > Suggested-by: David Spickett <david.spickett@linaro.org>
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
>
> Acked-by: Ard Biesheuvel <ardb@kernel.org>
>
> But can we add a bit more context to the commit log, please? It is not

Sure, that's a good idea.

> obvious to the casual reader why it is OK to emit UNPREDICTABLE
> opcodes, i.e., that these are selftests that aim to ensure that kprobe
> never attempts to replace the opcodes in question with a probe, but
> that they are not actually executed, or expected to occur in real
> code.

I'll add:

Ard notes:
  These are selftests that aim to ensure that kprobe
  never attempts to replace the opcodes in question with a probe, but
  they are not actually executed, or expected to occur in real
  code.

to the commit message, when submitting to RMK's queue.

Thanks for taking a look and the feedback.

>
> > ---
> >  arch/arm/probes/kprobes/test-arm.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/probes/kprobes/test-arm.c b/arch/arm/probes/kprobes/test-arm.c
> > index 977369f1aa48..2543106a203e 100644
> > --- a/arch/arm/probes/kprobes/test-arm.c
> > +++ b/arch/arm/probes/kprobes/test-arm.c
> > @@ -166,10 +166,10 @@ void kprobe_arm_test_cases(void)
> >
> >         /* Data-processing with PC as a target and status registers updated */
> >         TEST_UNSUPPORTED("movs  pc, r1")
> > -       TEST_UNSUPPORTED("movs  pc, r1, lsl r2")
> > +       TEST_UNSUPPORTED(__inst_arm(0xe1b0f211) "       @movs   pc, r1, lsl r2")
> >         TEST_UNSUPPORTED("movs  pc, #0x10000")
> >         TEST_UNSUPPORTED("adds  pc, lr, r1")
> > -       TEST_UNSUPPORTED("adds  pc, lr, r1, lsl r2")
> > +       TEST_UNSUPPORTED(__inst_arm(0xe09ef211) "       @adds   pc, lr, r1, lsl r2")
> >         TEST_UNSUPPORTED("adds  pc, lr, #4")
> >
> >         /* Data-processing with SP as target */
> > --
> > 2.30.0.478.g8a0d178c01-goog
> >
diff mbox series

Patch

diff --git a/arch/arm/probes/kprobes/test-arm.c b/arch/arm/probes/kprobes/test-arm.c
index 977369f1aa48..2543106a203e 100644
--- a/arch/arm/probes/kprobes/test-arm.c
+++ b/arch/arm/probes/kprobes/test-arm.c
@@ -166,10 +166,10 @@  void kprobe_arm_test_cases(void)
 
 	/* Data-processing with PC as a target and status registers updated */
 	TEST_UNSUPPORTED("movs	pc, r1")
-	TEST_UNSUPPORTED("movs	pc, r1, lsl r2")
+	TEST_UNSUPPORTED(__inst_arm(0xe1b0f211) "	@movs	pc, r1, lsl r2")
 	TEST_UNSUPPORTED("movs	pc, #0x10000")
 	TEST_UNSUPPORTED("adds	pc, lr, r1")
-	TEST_UNSUPPORTED("adds	pc, lr, r1, lsl r2")
+	TEST_UNSUPPORTED(__inst_arm(0xe09ef211) "	@adds	pc, lr, r1, lsl r2")
 	TEST_UNSUPPORTED("adds	pc, lr, #4")
 
 	/* Data-processing with SP as target */