Re: A collection of LD_AUDIT bugs that are important for tools (with better formatting for this list)

[Ben Woodard <woodard@redhat.com>]

> On Jul 30, 2021, at 7:58 AM, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
> 
> 
> 
> On 23/06/2021 14:42, Ben Woodard wrote:
>> 
>> 
>>> On Jun 17, 2021, at 4:06 PM, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
>>> 
>>> 
>>> 
>>> On 17/06/2021 17:09, Florian Weimer wrote:
>>>> * Adhemerval Zanella:
>>>> 
>>>>> 
>>>>> 
>>>>> * SVE support: as indicated by Szabolcs SVE calls are marked with the 
>>>>> STO_AARCH64_VARIANT_PCS and thus explicit not supported by dynamic loader. 
>> 
>> To me this sounds like partly a toolchain issue. The aarch64 PCS does define the ABI for SVE calls. I haven’t checked what GCC/binutils does in quite a while. It seems like the  STO_AARCH64_VARIANT_PCS was an expedient for SVE when it first came out and the semantics that it defines where all the registers are caller preserved makes it very difficult to implement around.
>> 
>> For LAV_CURRENT=2 what I planned to do was:
> 
> Now that I have finished the various audit issues that John Mellor-Crummey 
> has brought up, I think I have a better idea of how to address it.

Well the tier1 ones at least. Thank you. I was working on modifying John’s reproducers and making them official glibc tests so that when your  patch series lands in 2.35 they would have tests that ensure that no regressions show up.

I might not have seen it yet? Did you have a new version of 2/6 or a V3 version of the patch set which handles the lazy binding issue that you pointed out in. https://sourceware.org/pipermail/libc-alpha/2021-July/129510.html 

> 
>> 
>> diff --git a/sysdeps/aarch64/bits/link.h b/sysdeps/aarch64/bits/link.h
>> index ca76087ee1..390b12a826 100644
>> --- a/sysdeps/aarch64/bits/link.h
>> +++ b/sysdeps/aarch64/bits/link.h
>> @@ -20,13 +20,24 @@
>> # error "Never include <bits/link.h> directly; use <link.h> instead."
>> #endif
>> 
>> +typedef struct La_sve_regs {
>> +  uint16_t    *lr_preg[3];
>> +  __uint128_t *lr_zreg[8];
>> +} La_sve_regs;
>> +
>> /* Registers for entry into PLT on AArch64.  */
>> typedef struct La_aarch64_regs
>> {
>>   uint64_t    lr_xreg[9];
>> -  __uint128_t lr_vreg[8];
>>   uint64_t    lr_sp;
>>   uint64_t    lr_lr;
>> +  char    lr_sve; /* 0 - no SVE,
>> +                        1-16 length of the SVE registers in vq (128 bits) */
>> +  union {
>> +    /* when sve!=0 accessing the lr_vreg is undefined */
>> +    __uint128_t lr_vreg[8];
>> +    La_sve_regs lr_zreg;
>> +  };
>> } La_aarch64_regs;
>> 
>> /* Return values for calls from PLT on AArch64.  */
>> @@ -34,9 +45,14 @@ typedef struct La_aarch64_retval
>> {
>>   /* Up to eight integer registers can be used for a return value.  */
>>   uint64_t    lrv_xreg[8];
>> -  /* Up to eight V registers can be used for a return value.  */
>> -  __uint128_t lrv_vreg[8];
>> -
>> +  char        lrv_sve; /* 0 - no SVE,
>> +                         1-16 length of the SVE registers in vq (128 bits) */
>> +  union{
>> +    /* Up to eight V registers can be used for a return value.
>> +       When sve!=0 accessing the lr_vreg is undefined */
>> +    __uint128_t lrv_vreg[8];
>> +    La_sve_regs lrv_zreg;
>> +  };
>> } La_aarch64_retval;
>> __BEGIN_DECLS
> 
> My idea is to do something similar:
> ---
> typedef struct La_sve_regs
> {
>  uint16_t    *lr_preg;
>  long double *lr_zreg;
> } La_sve_regs;
> 
> typedef struct La_aarch64_regs
> {
>  [...]
>  uint8_t  lr_sve;           /* 0 - no SVE
>                                1-16 length of the SVE registers in vq (128 bits)  */
>  La_sve_regs *lr_sve_regs;  /* NULL - no SVE.  */
> };

Take this with a grain of salt you are the expert not me. These are three minor issues that I don’t really like about this approach:
1) I think having lr_sve be an uint8_t will mess up the alignment of the V registers that are immediately after it. It probably needs to be at the end of the structure or it needs to be padded out for alignment.
2) Second it appears like you have to traverse two pointers to actually get at the registers. In my opinion, it would be better if everything were just offsets from the beginning of the structure so that the actual memory for the Z and P registers is immediately following the La_aarch64_regs structure.
3) Don’t the V and the Z registers overlap in the machine architecture? Don’t we want to in some way overlap them so that you don’t have unused V register space in the structures when it is a SVE call?

What I would prefer to see in memory is:

  uint64_t lr_xreg[8];
  uint64_t lr_sp;
  uint64_t lr_lr;
  uint64_t  lr_sve;  // increased in size for alignment

then when lr_sve == 0
  long double lr_vreg[8];
or
  long double lr_zreg[8][lr_sve]
  uint16_t preg[4][lr_sve]
 
or something like that where the V or Z & P registers are immediately adjacent in memory to the rest of the normal registers. You don’t have to traverse pointers to get to them.

Could we have two la_aarch64_gnu_pltenter/exit functions? One which passes the normal La_aarch64_regs and one that passes la_aarch64_sveregs?

or would it be better to have two types, the normal version and then when lr_sve!=0 then you cast it to: La_aarch64_sve_regs which has the Z and the P regs instead.


> 
> typedef struct La_aarch64_retval
> {
>  [...]
>  uint8_t  lr_sve;           /* 0 - no SVE
>                                1-16 length of the SVE registers in vq (128 bits)  */
>  La_sve_regs *lr_sve_regs;  /* NULL - no SVE.  */
> }
> ---
> 
> The _dl_runtime_resolve will be responsible to allocate on the stack the required
> space for the La_sve_regs and setup the La_aarch64_regs and La_aarch64_retval internal
> pointers.  It has the advantage of allocate only the required space and if the we 
> can distinguish if the symbol does use SVE we can avoid the performance issue for 
> symbols that do not use SVE. The downside is it would require a potential large stack 
> space.
> 
>> 
>> However, that would require toolchain support and another hint in st_other which separates SVE calls from other uses of STO_AARCH64_VARIANT_PCS like STO_AARCH64_VARIANT_SVE. Then the runtime linker could populate the lrv_sve with information from the lrv_vreg with the size of the vector registers from the processor’s registers. 
> 
> I think it should be feasible to assume now that STO_AARCH64_VARIANT_PCS 
> means SVE, which meant that we can use it _dl_runtime_resolve to skip
> the save/restore if the symbol follows the default standard procedure
> call.  

Isn’t STO_AARCH64_VARIANT_PCS used for other kinds of calls as well, ones that require all the registers to be preserved.
I know that it would require some toolchain changes but shouldn’t we define something like STO_AARCH64_VARIANT_SVE to differentiate the two.

> 
>> 
>> There are at least two problems with that approach. 
>> 1) who allocates the lr_zreg pointers in the la_sve_regs and how long should they be? Do they always have to be allocated to be the max size 2048 bits?
>> 2) I hadn’t worked out how to handle functions that return things in the SVE regs. Do we need two new flags in st_other? STO_AARCH64_VARIANT_SVE and STO_AARCH64_VARIANT_SVERET?
> 
> I don't think we need the two extra flags to handle SVE calls, but I am
> also assuming that STO_AARCH64_VARIANT_PCS will be used solely for SVE.

Can you double check with your ARM contacts regarding that? I’m fairly sure that someone either in RH, ARM, or Linaro told me that VARIANT_PCS was also used other unusual situations other than SVE.

> And it has the extra problem of using two extra flags is not backward
> compatible.
> 
> I presume that if ARM wants to push for another procedure call variant
> on linux-gnu I would expect another flag.
> 
>> 
>> Then there was the question in the future could there be: big.LITTLE processors where some big processors had SVE registers of one length while the LITTLE processors had different ones? 
> 
> Although I find this kind of setup unlikely, my expectation is either that it
> would be transparent to userland (either kernel will emulate the required 
> instructions or it will bind process with STO_AARCH64_VARIANT_PCS to cores 
> with SVE).

Can you explain how that would work?

You launch a binary that has SVE from any processor. Somewhere along the way this means one of the exec syscalls handles it in the kernel. The kernel notices that it is an ELF file and hands it off to the ELF interpreter ld.so. Nowhere along the way would the kernel do a deep enough inspection of the binary to notice that it has SVE instructions in it and so how would it know to bind it to a processor which has HW SVE?

I also don’t see any places where ld.so can call back into the kernel and say “uh move me over to a processor that has these capabilities”. 

What I can imagine is a LITTLE processor traps into the kernel when executing a SVE instruction just like any illegal instruction. However, the handler looks at the instruction being executed notices it is an SVE instruction and instead of delivering a SIGILL to the process, it sets a flag in the task structure that says that this process uses SVE and migrates it to one of the big processors that can execute SVE.

-ben