diff mbox series

[2/2] aarch64: Set the syscall register right before doing the syscall.

Message ID 20230411133004.2268170-3-josimmon@redhat.com
State New
Headers show
Series x86_64: aarch64: Set call number just before syscall | expand

Commit Message

Joe Simmons-Talbott April 11, 2023, 1:30 p.m. UTC 
To make identifying syscalls easier during call tree analysis load the
syscall number just before performing the syscall.
---
 sysdeps/unix/sysv/linux/aarch64/sysdep.h | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Florian Weimer April 11, 2023, 1:50 p.m. UTC | #1 
* Joe Simmons-Talbott via Libc-alpha:

>    ({ long _sys_result;						\
>       {								\
>         LOAD_ARGS_##nr (args)					\
>         register long _x8 asm ("x8") = (name);			\
> +       if (__builtin_constant_p(name))				\
> +         asm volatile ("mov	x8, " MSTR(name) ";"		\
> +                       : /* no output */ : "i" (name) : "x8");	\
>         asm volatile ("svc	0	// syscall " # name     \
>  		     : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory");	\
>         _sys_result = _x0;					\

I think you should do this in a single assembler statement, load the
constant only once.

Thanks,
Florian
Adhemerval Zanella April 11, 2023, 2:15 p.m. UTC | #2 
On 11/04/23 10:50, Florian Weimer via Libc-alpha wrote:
> * Joe Simmons-Talbott via Libc-alpha:
> 
>>    ({ long _sys_result;						\
>>       {								\
>>         LOAD_ARGS_##nr (args)					\
>>         register long _x8 asm ("x8") = (name);			\
>> +       if (__builtin_constant_p(name))				\
>> +         asm volatile ("mov	x8, " MSTR(name) ";"		\
>> +                       : /* no output */ : "i" (name) : "x8");	\
>>         asm volatile ("svc	0	// syscall " # name     \
>>  		     : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory");	\
>>         _sys_result = _x0;					\
> 
> I think you should do this in a single assembler statement, load the
> constant only once.

Is this required because compiler is free to reorganize the argument
list? I think it should me it clear on the commit message.

Using a single assembler would require two inline asm, something like:

diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep.h b/sysdeps/unix/sysv/linux/aarch64/sysdep.h                                                                                            index e94d1703ad..2a128bb72d 100644                                                                                                                                                         --- a/sysdeps/unix/sysv/linux/aarch64/sysdep.h                                                                                                                                              +++ b/sysdeps/unix/sysv/linux/aarch64/sysdep.h                                                                                                                                              @@ -172,9 +172,19 @@
   ({ long _sys_result;                                         \
      {                                                         \
        LOAD_ARGS_##nr (args)                                   \
-       register long _x8 asm ("x8") = (name);                  \
-       asm volatile ("svc      0       // syscall " # name     \
-                    : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory"); \
+       if (__builtin_constant_p (name))                                \
+        asm volatile ("mov     x8, %1\n"                       \
+                      "svc     0       // syscall " # name     \
+                      : "=r" (_x0)                             \
+                      : "i" (name) ASM_ARGS_##nr               \
+                      : "x8", "memory");                       \
+       else                                                    \
+         {                                                     \
+          register long _x8 asm ("x8") = (name);               \
+          asm volatile ("svc      0       // syscall " # name  \
+                        : "=r" (_x0)                           \
+                        : "r"(_x8) ASM_ARGS_##nr : "memory");  \
+        }                                                      \
        _sys_result = _x0;                                      \
      }                                                         \
      _sys_result; })

Which really makes me doubt if this extra complexity is really necessary...
Szabolcs Nagy April 11, 2023, 3:43 p.m. UTC | #3 
The 04/11/2023 11:15, Adhemerval Zanella Netto via Libc-alpha wrote:
> On 11/04/23 10:50, Florian Weimer via Libc-alpha wrote:
> > * Joe Simmons-Talbott via Libc-alpha:
> > 
> >>    ({ long _sys_result;						\
> >>       {								\
> >>         LOAD_ARGS_##nr (args)					\
> >>         register long _x8 asm ("x8") = (name);			\
> >> +       if (__builtin_constant_p(name))				\
> >> +         asm volatile ("mov	x8, " MSTR(name) ";"		\
> >> +                       : /* no output */ : "i" (name) : "x8");	\
> >>         asm volatile ("svc	0	// syscall " # name     \
> >>  		     : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory");	\
> >>         _sys_result = _x0;					\
> > 
> > I think you should do this in a single assembler statement, load the
> > constant only once.
> 
> Is this required because compiler is free to reorganize the argument
> list? I think it should me it clear on the commit message.
> 
> Using a single assembler would require two inline asm, something like:

i don't like the use of __builtin_constant_name_p here.
if we want different behaviour for INTERNAL_SYSCALL_NCS
and other inlined syscall things then we should do the
dispatch there.

and yes it has to be one asm block since the compiler
is free to add random nops (or equivalent) anywhere.

i'm not entirely sure why this change is needed, a bit
more background would be useful.
Florian Weimer April 11, 2023, 4:03 p.m. UTC | #4 
* Adhemerval Zanella Netto:

> On 11/04/23 10:50, Florian Weimer via Libc-alpha wrote:
>> * Joe Simmons-Talbott via Libc-alpha:
>> 
>>>    ({ long _sys_result;						\
>>>       {								\
>>>         LOAD_ARGS_##nr (args)					\
>>>         register long _x8 asm ("x8") = (name);			\
>>> +       if (__builtin_constant_p(name))				\
>>> +         asm volatile ("mov	x8, " MSTR(name) ";"		\
>>> +                       : /* no output */ : "i" (name) : "x8");	\
>>>         asm volatile ("svc	0	// syscall " # name     \
>>>  		     : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory");	\
>>>         _sys_result = _x0;					\
>> 
>> I think you should do this in a single assembler statement, load the
>> constant only once.
>
> Is this required because compiler is free to reorganize the argument
> list? I think it should me it clear on the commit message.

Yes, that's the reason.  It's a bit tricky to recover the system call
number using static analysis otherwise.  I suggested to Joe that we
should put something into glibc, rather than improving that static
analysis tool so that it's fully reliable.

Thanks,
Florian
Adhemerval Zanella April 11, 2023, 4:39 p.m. UTC | #5 
On 11/04/23 13:03, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>> On 11/04/23 10:50, Florian Weimer via Libc-alpha wrote:
>>> * Joe Simmons-Talbott via Libc-alpha:
>>>
>>>>    ({ long _sys_result;						\
>>>>       {								\
>>>>         LOAD_ARGS_##nr (args)					\
>>>>         register long _x8 asm ("x8") = (name);			\
>>>> +       if (__builtin_constant_p(name))				\
>>>> +         asm volatile ("mov	x8, " MSTR(name) ";"		\
>>>> +                       : /* no output */ : "i" (name) : "x8");	\
>>>>         asm volatile ("svc	0	// syscall " # name     \
>>>>  		     : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory");	\
>>>>         _sys_result = _x0;					\
>>>
>>> I think you should do this in a single assembler statement, load the
>>> constant only once.
>>
>> Is this required because compiler is free to reorganize the argument
>> list? I think it should me it clear on the commit message.
> 
> Yes, that's the reason.  It's a bit tricky to recover the system call
> number using static analysis otherwise.  I suggested to Joe that we
> should put something into glibc, rather than improving that static
> analysis tool so that it's fully reliable.

Direct syscalls are done by different projects, like sanitizer, libgomp,
etc; so imho improving the static analysis tool could potentially catch
a wide range of usages than trying to fix only on glibc.
Joe Simmons-Talbott April 12, 2023, 3:27 p.m. UTC | #6 
On Tue, Apr 11, 2023 at 01:39:31PM -0300, Adhemerval Zanella Netto wrote:
> 
> 
> On 11/04/23 13:03, Florian Weimer wrote:
> > * Adhemerval Zanella Netto:
> > 
> >> On 11/04/23 10:50, Florian Weimer via Libc-alpha wrote:
> >>> * Joe Simmons-Talbott via Libc-alpha:
> >>>
> >>>>    ({ long _sys_result;						\
> >>>>       {								\
> >>>>         LOAD_ARGS_##nr (args)					\
> >>>>         register long _x8 asm ("x8") = (name);			\
> >>>> +       if (__builtin_constant_p(name))				\
> >>>> +         asm volatile ("mov	x8, " MSTR(name) ";"		\
> >>>> +                       : /* no output */ : "i" (name) : "x8");	\
> >>>>         asm volatile ("svc	0	// syscall " # name     \
> >>>>  		     : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory");	\
> >>>>         _sys_result = _x0;					\
> >>>
> >>> I think you should do this in a single assembler statement, load the
> >>> constant only once.
> >>
> >> Is this required because compiler is free to reorganize the argument
> >> list? I think it should me it clear on the commit message.
> > 
> > Yes, that's the reason.  It's a bit tricky to recover the system call
> > number using static analysis otherwise.  I suggested to Joe that we
> > should put something into glibc, rather than improving that static
> > analysis tool so that it's fully reliable.
> 
> Direct syscalls are done by different projects, like sanitizer, libgomp,
> etc; so imho improving the static analysis tool could potentially catch
> a wide range of usages than trying to fix only on glibc.
> 

I agree that improving the static analysis tool would be helpful.  One
thing to keep in mind is that this patchset will also aid people doing
static analysis manually.  It also seems likely that other static
analysis tools would benefit from this change.

Thanks,
Joe
diff mbox series

Patch

diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep.h b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
index e94d1703ad..42a70feac2 100644
--- a/sysdeps/unix/sysv/linux/aarch64/sysdep.h
+++ b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
@@ -167,12 +167,18 @@ 
 
 # define HAVE_CLONE3_WRAPPER		1
 
+# define MSTR_HELPER(x) # x
+# define MSTR(x) MSTR_HELPER(x)
+
 # undef INTERNAL_SYSCALL_RAW
 # define INTERNAL_SYSCALL_RAW(name, nr, args...)		\
   ({ long _sys_result;						\
      {								\
        LOAD_ARGS_##nr (args)					\
        register long _x8 asm ("x8") = (name);			\
+       if (__builtin_constant_p(name))				\
+         asm volatile ("mov	x8, " MSTR(name) ";"		\
+                       : /* no output */ : "i" (name) : "x8");	\
        asm volatile ("svc	0	// syscall " # name     \
 		     : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory");	\
        _sys_result = _x0;					\