Re: [PATCH v2 for 5.2 08/12] rseq/selftests: arm: use udf instruction for RSEQ_SIG

----- On Jun 6, 2019, at 8:02 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:

----- On May 3, 2019, at 3:38 PM, Mathieu Desnoyers
mathieu.desnoyers@efficios.com wrote:

quoted

Use udf as the guard instruction for the restartable sequence abort
handler.

Previously, the chosen signature was not a valid instruction, based
on the assumption that it could always sit in a literal pool. However,
there are compilation environments in which literal pools are not
available, for instance execute-only code. Therefore, we need to
choose a signature value that is also a valid instruction.

Handle compiling with -mbig-endian on ARMv6+, which generates binaries
with mixed code vs data endianness (little endian code, big endian
data).

Else mismatch between code endianness for the generated signatures and
data endianness for the RSEQ_SIG parameter passed to the rseq
registration will trigger application segmentation faults when the
kernel try to abort rseq critical sections.

Prior to ARMv6, -mbig-endian generates big-endian code and data, so
endianness should not be reversed in that case.

And of course it cannot be that easy. This breaks when building in
thumb mode (-mthumb). Output from librseq arm32 build [1] (code similar
to what is found in the rseq selftests):

 CC       rseq.lo
/tmp/ccu6Jw1b.s: Assembler messages:
/tmp/ccu6Jw1b.s:297: Error: cannot determine Thumb instruction size. Use
.inst.n/.inst.w instead
/tmp/ccu6Jw1b.s:490: Error: cannot determine Thumb instruction size. Use
.inst.n/.inst.w instead
Makefile:460: recipe for target 'rseq.lo' failed

This appears to be caused by a missing .arm directive in RSEQ_SIG_DATA.
Fixing with:

-               asm volatile ("b 2f\n\t"                                \
+               asm volatile (".arm\n\t"                                \
+                             "b 2f\n\t"                                \

gets the build to go further, but breaks at:

 CC       basic_percpu_ops_test.o
/tmp/ccpHOMHZ.s: Assembler messages:
/tmp/ccpHOMHZ.s:148: Error: misaligned branch destination
/tmp/ccpHOMHZ.s:956: Error: misaligned branch destination
Makefile:378: recipe for target 'basic_percpu_ops_test.o' failed

I suspect it's caused by the change from:

-               ".word " __rseq_str(RSEQ_SIG) "\n\t"                    \

to

+               ".arm\n\t"                                              \
+               ".inst " __rseq_str(RSEQ_SIG_CODE) "\n\t"               \

which changes the mode from thumb to arm for the rest of the
inline asm within __RSEQ_ASM_DEFINE_ABORT. Better yet, there appears
to be no way to save the arm/thumb state and restore it afterwards.

I'm really starting to wonder if we should go our of our way to try
to get this signature to be a valid instruction on arm32. Perhaps
we should consider going back to use ".word" on arm32 so it ensures
it uses data endianness (which matches the parameter received by the
sys_rseq system call), let objdump and friends print it as a literal
pool (which it is), and just choose an instruction which has little
chances to appear for the cases we care about between ARM32 BE, LE
and THUMB. Perhaps a 32-bit palindrome ? Bonus points if this is a
trap instruction in common configurations for odd-cases-debugging
purposes.

So I'm not particularly proud of the result, but I found a rather
ugly way to figure out if we are currently in thumb mode within an
inline asm, and restore that mode: test the length of a nop
instruction with a ".if" asm statement.

Do we want to go for this kind of approach, or should we revert
back to a ".word" and accept that the rseq signature before the
abort handler will be seen as data rather than an instruction
on arm32 ?

Is there a better way to do this ?

Thanks,

Mathieu

diff --git a/include/rseq/rseq-arm.h b/include/rseq/rseq-arm.h
index 1ce9231..b6c36dd 100644
--- a/include/rseq/rseq-arm.h
+++ b/include/rseq/rseq-arm.h

@@ -43,7 +43,14 @@
        ({                                                              \
                int sig;                                                \
                asm volatile ("b 2f\n\t"                                \
+                             "3:\n\t"                                  \
+                             "nop\n\t"                                 \
+                             "4:\n\t"                                  \
+                             ".arm\n\t"                                \
                              "1: .inst " __rseq_str(RSEQ_SIG_CODE) "\n\t" \
+                             ".if ((4b - 3b) == 2)\n\t"                \
+                             ".thumb\n\t"                              \
+                             ".endif\n\t"                              \
                              "2:\n\t"                                  \
                              "ldr %[sig], 1b\n\t"                      \
                              : [sig] "=r" (sig));                      \

@@ -125,8 +132,14 @@ do {                                                                       \
                __rseq_str(table_label) ":\n\t"                         \
                ".word " __rseq_str(version) ", " __rseq_str(flags) "\n\t" \
                ".word " __rseq_str(start_ip) ", 0x0, " __rseq_str(post_commit_offset) ", 0x0, " __rseq_str(abort_ip) ", 0x0\n\t" \
+               "333:\n\t"                                              \
+               "nop\n\t"                                               \
+               "444:\n\t"                                              \
                ".arm\n\t"                                              \
                ".inst " __rseq_str(RSEQ_SIG_CODE) "\n\t"               \
+               ".if ((444b - 333b) == 2)\n\t"                          \
+               ".thumb\n\t"                                            \
+               ".endif\n\t"                                            \
                __rseq_str(label) ":\n\t"                               \
                teardown                                                \
                "b %l[" __rseq_str(abort_label) "]\n\t"

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com