RFC La_aarch64_regs and LD_AUDIT

RFC La_aarch64_regs and LD_AUDIT

[Ben Coyote Woodard]

A user reported to me that LD_AUDIT wasn't working properly on aarch64. 
We tracked down the problem to the fact that x8 the indirect result 
location register was not being saved in _dl_runtime_profile. At the 
same time we observed that in _dl_runtime_profile only saved half the 
NEON registers.  It appears like the code was copied over from ARMv7 
before the vector registers were 128b and was not updated when 
_dl_runtime_resolve was updated to aarch64's ABI.

To fix these two issues I had to break ABI and change the 
La_aarch64_regs structure to match the AAPCS for aarch64 . 
<https://developer.arm.com/documentation/ihi0042/j/> The structure went 
from:

typedef struct La_aarch64_regs
{
  uint64_t lr_xreg[8];
  uint64_t lr_dreg[8];
  uint64_t lr_sp;
  uint64_t lr_lr;
} La_aarch64_regs;

to:

typedef struct La_aarch64_regs
{
  uint64_t    lr_xreg[9];
  __uint128_t lr_vreg[8];
  uint64_t    lr_sp;
  uint64_t    lr_lr;
} La_aarch64_regs;

I consider this ABI break to be unavoidable. However there is already a 
revision to the AAPCS for processors with SVE 
<https://developer.arm.com/documentation/100986/0000/?search=5eec7447e24a5e02d07b2774> 
There is already a processor commercially available with SVE support 
the A64FX from Fujitsu and more are on the way in the next few years. 
Obviously, even the updated La_aarch64_regs structure doesn't have 
space for the SVE registers that the specification requires. So as to 
not break ABI a second time, we should update the La_aarch64_regs 
structure to support the SVE version of the procedure call standard but 
how?

The naive approach would be to make the La_aarch64_regs structure 
handle the full sized SVE registers something like:

ypedef struct La_aarch64_regs
{
  uint64_t    lr_xreg[9];
  union {
  __uint128 lr_vreg[8];
  char lr_zreg[256][8];
  };
  char lr_preg[8][3];
  uint64_t    lr_sp;
  uint64_t    lr_lr;
} La_aarch64_regs;

However, that seems extremely wasteful over the short term because it 
would lead to register structures over 2K when the most common case is 
going to be that SVE registers are seldom used except in very specific 
cases. Therefore how do we want to handle this?

One idea that I had is:
typedef struct La_sve_unit
{
  __uint128_t lr_zreg[8];
  uint16_t    lr_preg[3];
}; /* 134 bytes in size */

typedef struct La_aarch64_regs
{
  unsigned int sve_len:8;
  uint64_t lr_xreg[9];
  uint64_t lr_sp;
  uint64_t lr_lr;
  union {
    __uint128_t        lr_vreg[8];
    struct La_sve_unit lr_sve_reg[1];
  };
} La_aarch64_regs;

Then defining some additional structures like:

typedef struct La_aarch64_regs_SVE2
{
  unsigned int sve_len:8;
  uint64_t lr_xreg[9];
  uint64_t lr_sp;
  uint64_t lr_lr;
  union {
    __uint128_t        lr_vreg[8];
    struct La_sve_chunk lr_sve_reg[2];
  };
} La_aarch64_regs_SVE2;

and La_aarch64_regs_SVE4 that a program calling la_aarch64_gnu_pltenter 
 could cast its La_aarch64_regs into based upon the size of the sve_len 
on that processor's implementation of SVE.

I don't know what is the right thing to do about SVE is and I think 
that this should be hashed out before we change structure of 
La_aarch64_regs and break ABI even if it is to fix a problem that tools 
authors are already running into.

Anyway a patch to fix LD_AUDIT for non-SVE processors can be found in:
<https://sourceware.org/bugzilla/show_bug.cgi?id=26643>

-ben

Re: RFC La_aarch64_regs and LD_AUDIT

[Florian Weimer]

* Ben Coyote Woodard via Libc-alpha:

> To fix these two issues I had to break ABI and change the
> La_aarch64_regs structure to match the AAPCS for aarch64 . 
> <https://developer.arm.com/documentation/ihi0042/j/> The structure
> went from:
>
> typedef struct La_aarch64_regs
> {
>  uint64_t lr_xreg[8];
>  uint64_t lr_dreg[8];
>  uint64_t lr_sp;
>  uint64_t lr_lr;
> } La_aarch64_regs;
>
> to:
>
> typedef struct La_aarch64_regs
> {
>  uint64_t    lr_xreg[9];
>  __uint128_t lr_vreg[8];
>  uint64_t    lr_sp;
>  uint64_t    lr_lr;
> } La_aarch64_regs;
>
> I consider this ABI break to be unavoidable.

There is no ABI break as such if the dynamic linker checks the version
returned by la_version and we increase the LAV_CURRENT value.  Existing
audit modules continue to work.

> I don't know what is the right thing to do about SVE is and I think
> that this should be hashed out before we change structure of 
> La_aarch64_regs and break ABI even if it is to fix a problem that
> tools authors are already running into.

My concern is that we should try to come up with a way to use processor
support to save the context, so that we can avoid future LAV_CURRENT
bumps if processor properties related to SVE change.  Ideally, the
dynamic loader would just pass through the data, and the audit module
can use this data using whatever backwards/forwards compatibility
features the ABI offers.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: RFC La_aarch64_regs and LD_AUDIT

[Carlos O'Donell]

On 9/22/20 1:45 PM, Ben Coyote Woodard via Libc-alpha wrote:
> Anyway a patch to fix LD_AUDIT for non-SVE processors can be found in:
> <https://sourceware.org/bugzilla/show_bug.cgi?id=26643>

Please post patches to the list.

We have updated the contribution checklist to make this easier:
https://sourceware.org/glibc/wiki/Contribution%20checklist

That way we can talk about the specific patch on the list.

-- 
Cheers,
Carlos.

Re: RFC La_aarch64_regs and LD_AUDIT

[Carlos O'Donell]

On 9/22/20 2:11 PM, Florian Weimer via Libc-alpha wrote:
> * Ben Coyote Woodard via Libc-alpha:
> 
>> To fix these two issues I had to break ABI and change the
>> La_aarch64_regs structure to match the AAPCS for aarch64 . 
>> <https://developer.arm.com/documentation/ihi0042/j/> The structure
>> went from:
>>
>> typedef struct La_aarch64_regs
>> {
>>  uint64_t lr_xreg[8];
>>  uint64_t lr_dreg[8];
>>  uint64_t lr_sp;
>>  uint64_t lr_lr;
>> } La_aarch64_regs;
>>
>> to:
>>
>> typedef struct La_aarch64_regs
>> {
>>  uint64_t    lr_xreg[9];
>>  __uint128_t lr_vreg[8];
>>  uint64_t    lr_sp;
>>  uint64_t    lr_lr;
>> } La_aarch64_regs;
>>
>> I consider this ABI break to be unavoidable.
> 
> There is no ABI break as such if the dynamic linker checks the version
> returned by la_version and we increase the LAV_CURRENT value.  Existing
> audit modules continue to work.

Correct, but it requires more code in the loader to handle that.

It isn't entirely clear in Solaris what happens if you return a value
from la_version() that is lower than LAV_CURRENT. I expect the dynamic
loader gets to decide what to do in each case, either discarding the
auditor or implementing the requested interface.

You could have a mix of audit modules of different versions, track their
versions, and copy around structures before calling la_pltenter and
la_pltexit, being careful to implement the interface as requested by
the auditor.

That is to say that elf/dl-runtime.c would need a per-arch callout to
determine actions to take based on the auditors version.

It is possible to support this without ABI breaks, but it needs more
work.

>> I don't know what is the right thing to do about SVE is and I think
>> that this should be hashed out before we change structure of 
>> La_aarch64_regs and break ABI even if it is to fix a problem that
>> tools authors are already running into.
> 
> My concern is that we should try to come up with a way to use processor
> support to save the context, so that we can avoid future LAV_CURRENT
> bumps if processor properties related to SVE change.  Ideally, the
> dynamic loader would just pass through the data, and the audit module
> can use this data using whatever backwards/forwards compatibility
> features the ABI offers.

I agree that would be optimal.

-- 
Cheers,
Carlos.

Re: RFC La_aarch64_regs and LD_AUDIT

[Ben Coyote Woodard]

On Tue, Sep 22, 2020 at 14:24, Carlos O'Donell via Libc-alpha 
<libc-alpha@sourceware.org> wrote:
> On 9/22/20 2:11 PM, Florian Weimer via Libc-alpha wrote:
>>  * Ben Coyote Woodard via Libc-alpha:
>> 
>>>  To fix these two issues I had to break ABI and change the
>>>  La_aarch64_regs structure to match the AAPCS for aarch64 .
>>>  <<https://developer.arm.com/documentation/ihi0042/j/>> The 
>>> structure
>>>  went from:
>>> 
>>>  typedef struct La_aarch64_regs
>>>  {
>>>   uint64_t lr_xreg[8];
>>>   uint64_t lr_dreg[8];
>>>   uint64_t lr_sp;
>>>   uint64_t lr_lr;
>>>  } La_aarch64_regs;
>>> 
>>>  to:
>>> 
>>>  typedef struct La_aarch64_regs
>>>  {
>>>   uint64_t    lr_xreg[9];
>>>   __uint128_t lr_vreg[8];
>>>   uint64_t    lr_sp;
>>>   uint64_t    lr_lr;
>>>  } La_aarch64_regs;
>>> 
>>>  I consider this ABI break to be unavoidable.
>> 
>>  There is no ABI break as such if the dynamic linker checks the 
>> version
>>  returned by la_version and we increase the LAV_CURRENT value.  
>> Existing
>>  audit modules continue to work.
> 
> Correct, but it requires more code in the loader to handle that.
> 
> It isn't entirely clear in Solaris what happens if you return a value
> from la_version() that is lower than LAV_CURRENT. I expect the dynamic
> loader gets to decide what to do in each case, either discarding the
> auditor or implementing the requested interface.

I'm in favor of:  The dynamic linker prints out a message which says 
something to the effect of:
Auditor interface version of <libaudit.so> is not compatible with with 
the interface provided by this version of GLibc.

and then it terminates. LD_AUDIT libraries are exceedingly rare and the 
people who use them are extremely sophisticated tool developers who can 
update and recompile their audit libraries. This isn't a place where 
there is much value add in adding the complexity of supporting previous 
interface versions. Keep in mind that this interface was broken to the 
point where it is unusable for the past 8 years and this is the first 
time someone noticed it.

Furthermore, in quite a few cases it is rather likely bumping 
LAV_CURRENT will be practically ineffective anyway. As a case in point 
the reproducer code which came with the bug report simply returns the 
version of the interface that was passed into it as is mentioned in the 
rtld-audit man page. So code will break anyway unless they recompile. 
(Being somewhat facetious here: Don't worry I have the personal cell 
phone number of the only two tool developers in the world who will be 
impacted by changing the ABI of plt enter/exit interface of rtld-audit 
on aarch64. One reported the bug to me and the other was cc'd in the 
original report ;-) )

> 
> You could have a mix of audit modules of different versions, track 
> their
> versions, and copy around structures before calling la_pltenter and
> la_pltexit, being careful to implement the interface as requested by
> the auditor.
> 
> That is to say that elf/dl-runtime.c would need a per-arch callout to
> determine actions to take based on the auditors version.
> 
> It is possible to support this without ABI breaks, but it needs more
> work.
> 
>>>  I don't know what is the right thing to do about SVE is and I think
>>>  that this should be hashed out before we change structure of
>>>  La_aarch64_regs and break ABI even if it is to fix a problem that
>>>  tools authors are already running into.
>> 
>>  My concern is that we should try to come up with a way to use 
>> processor
>>  support to save the context, so that we can avoid future LAV_CURRENT
>>  bumps if processor properties related to SVE change.  Ideally, the
>>  dynamic loader would just pass through the data, and the audit 
>> module
>>  can use this data using whatever backwards/forwards compatibility
>>  features the ABI offers.
> 
> I agree that would be optimal.

> 
> --
> Cheers,
> Carlos.
>