[PATCH] rtld-audit.7: Clarify la_version handshake

[PATCH] rtld-audit.7: Clarify la_version handshake

[Florian Weimer]

Returning its argument without further checks is almost always
wrong for la_version.

Signed-off-by: Florian Weimer <fweimer@redhat.com>

---
 man7/rtld-audit.7 | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/man7/rtld-audit.7 b/man7/rtld-audit.7
index b1b7dfebc..ca8afa752 100644
--- a/man7/rtld-audit.7
+++ b/man7/rtld-audit.7
@@ -70,17 +70,30 @@ the auditing library.
 When invoking this function, the dynamic linker passes, in
 .IR version ,
 the highest version of the auditing interface that the linker supports.
-If necessary, the auditing library can check that this version
-is sufficient for its requirements.
 .PP
-As its function result,
-this function should return the version of the auditing interface
-that this auditing library expects to use (returning
+A typical implementation of this function simply returns the constant
+.BR LAV_CURRENT ,
+which indicates the version of
+.I <link.h>
+that was used to build the audit module.  If the dynamic linker does
+not support this version of the audit interface, it will refuse to
+activate this audit module.  If the function returns zero, the dynamic
+linker also does not activate this audit module.
+.PP
+In order to enable backwards compatibility with older dynamic linkers,
+an audit module can examine the
+.I version
+argument and return an earlier version than
+.BR LAV_CURRENT ,
+assuming the module can adjust its implement to match the requirements
+of the previous version of the audit interface.  The
+.B la_version
+function should not return the value of
 .I version
-is acceptable).
-If the returned value is 0,
-or a version that is greater than that supported by the dynamic linker,
-then the audit library is ignored.
+without further checks because it could correspond to an interface
+that does not match the
+.I <link.h>
+definitions used to build the audit module.
 .SS la_objsearch()
 \&
 .nf

-- 
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: [PATCH] rtld-audit.7: Clarify la_version handshake

[Andreas Schwab]

On Sep 23 2020, Florian Weimer via Libc-alpha wrote:

> +.PP
> +In order to enable backwards compatibility with older dynamic linkers,
> +an audit module can examine the
> +.I version
> +argument and return an earlier version than
> +.BR LAV_CURRENT ,
> +assuming the module can adjust its implement to match the requirements

s/implement/interface/ ?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] rtld-audit.7: Clarify la_version handshake

[Florian Weimer]

* Andreas Schwab:

> On Sep 23 2020, Florian Weimer via Libc-alpha wrote:
>
>> +.PP
>> +In order to enable backwards compatibility with older dynamic linkers,
>> +an audit module can examine the
>> +.I version
>> +argument and return an earlier version than
>> +.BR LAV_CURRENT ,
>> +assuming the module can adjust its implement to match the requirements
>
> s/implement/interface/ ?

Thanks.  I meant to write “implementation”.  Fixed locally.

Do you agree that this is how the handshake should work?

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: [PATCH] rtld-audit.7: Clarify la_version handshake

[Andreas Schwab]

On Sep 23 2020, Florian Weimer wrote:

> Do you agree that this is how the handshake should work?

I have never seen the audit interface being used, so I have no opinion.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] rtld-audit.7: Clarify la_version handshake

[Ben Coyote Woodard]

I think that you are correctly characterizing the original intent of the 
solaris developers.

However, I believe that their fundamental design was a poor design. 
Doing this kind of magic number handshake and then imbuing a number with 
some deeper semantic meaning that exists outside of the interface itself 
is a bad design and prone to errors over the long run.

Then there is the problem that you have a pair of special functions for 
each architecture la_pltenter() and la_pltexit() which are dependent on 
the size and layout of structures in that are implied by the same 
interface number.

This is what I would do:

1) require that audit libs be compiled with -g. "No DWARF no worky"
2) Iterate through all the la_* functions in the audit library and 
compare the DWARF for their function declarations to the DWARF from the 
function prototypes that defined the calls that you make in the runtime 
linker. This would also mean that the types types for the parameters to 
these functions would be checked. That way you would be able to detect 
if something like La_*_regs changed which is the problem that you have 
with ABI variations.
3) Because you are checking the parameter types for the la_* functions, 
the types for the preserved registers and the return values could vary 
across architectures with no conflict. Just make the structures La_regs 
and La_retval.
   a) or backward compatibility typedef La_<arch>_regs and 
La_<arch>_retval if you like.
4) The fact that you have the same named parameter types for all the 
architectures allows you to get rid of the architecturally specific 
versions of la_pltenter and la_pltexit.
5) Switch to C++ name mangling then and then the differences between the 
32b and 64b versions of the interface can be implemented with a template 
instantiation.
6) Changing to C++ name mangled interfaces would also allow us to deal 
with things like SVE or ABI changes more easily. For example:
    For ARM there would be two overloaded la_pltenter() and la_pltexit() 
calls. One would be:

   la_pltenter (ElfW(Sym) *__sym, unsigned int __ndx,
              uintptr_t *__refcook,
              uintptr_t *__defcook,
              La_regs *__regs,
              unsigned int *__flags,
              const char *__symname,
              long int *__framesizep);

and the other would be:

   la_pltenter (ElfW(Sym) *__sym, unsigned int __ndx,
              uintptr_t *__refcook,
              uintptr_t *__defcook,
              La_sve_regs *__regs, // <- different type here
              unsigned int *__flags,
              const char *__symname,
              long int *__framesizep);

Then the handler for STO_AARCH64_VARIANT_PCS could be wired up to call 
the SVE version of la_pltenter and la_pltexit. The same sort of trick 
could be used for architecture ABI breaks. Having the handler key off of 
the ELF ABI version.

The overall point is we can do so much better now. Pedantically, 
adhering to a crufty 40 year old interface what was not well thought 
through to begin with and which hasn't been refined because so few 
people use it, is really not a good way to ensure that GNU/Linux 
continues to be viable into the future.

-ben


On 9/23/20 4:38 AM, Florian Weimer via Libc-alpha wrote:
> Returning its argument without further checks is almost always
> wrong for la_version.
>
> Signed-off-by: Florian Weimer <fweimer@redhat.com>
>
> ---
>   man7/rtld-audit.7 | 31 ++++++++++++++++++++++---------
>   1 file changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/man7/rtld-audit.7 b/man7/rtld-audit.7
> index b1b7dfebc..ca8afa752 100644
> --- a/man7/rtld-audit.7
> +++ b/man7/rtld-audit.7
> @@ -70,17 +70,30 @@ the auditing library.
>   When invoking this function, the dynamic linker passes, in
>   .IR version ,
>   the highest version of the auditing interface that the linker supports.
> -If necessary, the auditing library can check that this version
> -is sufficient for its requirements.
>   .PP
> -As its function result,
> -this function should return the version of the auditing interface
> -that this auditing library expects to use (returning
> +A typical implementation of this function simply returns the constant
> +.BR LAV_CURRENT ,
> +which indicates the version of
> +.I <link.h>
> +that was used to build the audit module.  If the dynamic linker does
> +not support this version of the audit interface, it will refuse to
> +activate this audit module.  If the function returns zero, the dynamic
> +linker also does not activate this audit module.
> +.PP
> +In order to enable backwards compatibility with older dynamic linkers,
> +an audit module can examine the
> +.I version
> +argument and return an earlier version than
> +.BR LAV_CURRENT ,
> +assuming the module can adjust its implement to match the requirements
> +of the previous version of the audit interface.  The
> +.B la_version
> +function should not return the value of
>   .I version
> -is acceptable).
> -If the returned value is 0,
> -or a version that is greater than that supported by the dynamic linker,
> -then the audit library is ignored.
> +without further checks because it could correspond to an interface
> +that does not match the
> +.I <link.h>
> +definitions used to build the audit module.
>   .SS la_objsearch()
>   \&
>   .nf
>

Re: [PATCH] rtld-audit.7: Clarify la_version handshake

[Carlos O'Donell]

On 9/23/20 7:38 AM, Florian Weimer via Libc-alpha wrote:
> Returning its argument without further checks is almost always
> wrong for la_version.

What you write here is the way I understand the interface to work.

As Andreas noted there is one typo.

I suggest changing the example to match, otherwise people might
keep cribbing that code ;-)
 
> Signed-off-by: Florian Weimer <fweimer@redhat.com>
> 

I agree that la_version should return LAV_CURRENT, that is the
expected way to implement the function, and returning less than
LAV_CURRENT is required to implement older interfaces and be
more compatible with older releases.

As an example I encouraged Ludovic to solve a Guix path problem
with LD_AUDIT and you can see it here:
https://issues.guix.gnu.org/41189

+unsigned int
+la_version (unsigned int v)
+{
+  if (v != LAV_CURRENT)
+    error (1, 0, "cannot handle interface version %u", v);
+
...
+  return v;
+}

While this is OK, in that if it's not an exact match the audit
module errors out, or if it is a match returns v which is
LAV_CURRENT.

It could have been more relaxed:

if (v < LAV_CURRENT)
  error (...)
...
return LAV_CURRENT;

Your changes here int he man page would have clarified that to
some extent.

> ---
>  man7/rtld-audit.7 | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/man7/rtld-audit.7 b/man7/rtld-audit.7
> index b1b7dfebc..ca8afa752 100644
> --- a/man7/rtld-audit.7
> +++ b/man7/rtld-audit.7
> @@ -70,17 +70,30 @@ the auditing library.
>  When invoking this function, the dynamic linker passes, in
>  .IR version ,
>  the highest version of the auditing interface that the linker supports.
> -If necessary, the auditing library can check that this version
> -is sufficient for its requirements.
>  .PP
> -As its function result,
> -this function should return the version of the auditing interface
> -that this auditing library expects to use (returning
> +A typical implementation of this function simply returns the constant
> +.BR LAV_CURRENT ,
> +which indicates the version of
> +.I <link.h>
> +that was used to build the audit module.  If the dynamic linker does
> +not support this version of the audit interface, it will refuse to
> +activate this audit module.  If the function returns zero, the dynamic
> +linker also does not activate this audit module.
> +.PP
> +In order to enable backwards compatibility with older dynamic linkers,
> +an audit module can examine the
> +.I version
> +argument and return an earlier version than
> +.BR LAV_CURRENT ,
> +assuming the module can adjust its implement to match the requirements

s/implement/implementation/g (Andreas caught this in his review)

> +of the previous version of the audit interface.  The
> +.B la_version
> +function should not return the value of
>  .I version
> -is acceptable).
> -If the returned value is 0,
> -or a version that is greater than that supported by the dynamic linker,
> -then the audit library is ignored.
> +without further checks because it could correspond to an interface
> +that does not match the
> +.I <link.h>
> +definitions used to build the audit module.
>  .SS la_objsearch()
>  \&
>  .nf
> 

May I additionally suggest something like this to the example?

diff --git a/man7/rtld-audit.7 b/man7/rtld-audit.7
index b1b7dfebc..8a4008b2a 100644
--- a/man7/rtld-audit.7
+++ b/man7/rtld-audit.7
@@ -510,7 +510,11 @@ la_version(unsigned int version)
 {
     printf("la_version(): %u\en", version);
 
-    return version;
+    if (version > LAV_CURRENT)
+      printf ("la_version(): rtld supports newer interfaces.\n");
+
+    /* Request the version we knew about at compile time.  */
+    return LAV_CURRENT;
 }
 
 char *
---

Post a v2?

-- 
Cheers,
Carlos.

Re: [PATCH] rtld-audit.7: Clarify la_version handshake

[Florian Weimer]

* Carlos O'Donell:

> May I additionally suggest something like this to the example?

Ah, I had missed the example at the end.  Yes, I'm going to send a v2.

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

[PATCH] rtld-audit.7: Clarify la_version handshake

[Florian Weimer]

Returning its argument without further checks is almost always
wrong for la_version.

Signed-off-by: Florian Weimer <fweimer@redhat.com>

---
 man7/rtld-audit.7 | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/man7/rtld-audit.7 b/man7/rtld-audit.7
index b1b7dfebc..21cc339d0 100644
--- a/man7/rtld-audit.7
+++ b/man7/rtld-audit.7
@@ -70,17 +70,30 @@ the auditing library.
 When invoking this function, the dynamic linker passes, in
 .IR version ,
 the highest version of the auditing interface that the linker supports.
-If necessary, the auditing library can check that this version
-is sufficient for its requirements.
 .PP
-As its function result,
-this function should return the version of the auditing interface
-that this auditing library expects to use (returning
+A typical implementation of this function simply returns the constant
+.BR LAV_CURRENT ,
+which indicates the version of
+.I <link.h>
+that was used to build the audit module.  If the dynamic linker does
+not support this version of the audit interface, it will refuse to
+activate this audit module.  If the function returns zero, the dynamic
+linker also does not activate this audit module.
+.PP
+In order to enable backwards compatibility with older dynamic linkers,
+an audit module can examine the
+.I version
+argument and return an earlier version than
+.BR LAV_CURRENT ,
+assuming the module can adjust its implementation to match the
+requirements of the previous version of the audit interface.  The
+.B la_version
+function should not return the value of
 .I version
-is acceptable).
-If the returned value is 0,
-or a version that is greater than that supported by the dynamic linker,
-then the audit library is ignored.
+without further checks because it could correspond to an interface
+that does not match the
+.I <link.h>
+definitions used to build the audit module.
 .SS la_objsearch()
 \&
 .nf
@@ -508,9 +521,10 @@ This is reportedly fixed in glibc 2.10.
 unsigned int
 la_version(unsigned int version)
 {
-    printf("la_version(): %u\en", version);
+    printf("la_version(): version = %u; LAV_CURRENT = %u\en",
+            version, LAV_CURRENT);
 
-    return version;
+    return LAV_CURRENT;
 }
 
 char *

-- 
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: [PATCH] rtld-audit.7: Clarify la_version handshake

[Michael Kerrisk (man-pages)]

On 9/25/20 10:48 AM, Florian Weimer wrote:
> Returning its argument without further checks is almost always
> wrong for la_version.
> 
> Signed-off-by: Florian Weimer <fweimer@redhat.com>

Hello Florian,

I've applied this patch locally. I'll merge into master
in a few hours. Perhaps in the meantime there are acks/reviews
that come in.

Cheers,

Michael

> ---
>  man7/rtld-audit.7 | 36 +++++++++++++++++++++++++-----------
>  1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/man7/rtld-audit.7 b/man7/rtld-audit.7
> index b1b7dfebc..21cc339d0 100644
> --- a/man7/rtld-audit.7
> +++ b/man7/rtld-audit.7
> @@ -70,17 +70,30 @@ the auditing library.
>  When invoking this function, the dynamic linker passes, in
>  .IR version ,
>  the highest version of the auditing interface that the linker supports.
> -If necessary, the auditing library can check that this version
> -is sufficient for its requirements.
>  .PP
> -As its function result,
> -this function should return the version of the auditing interface
> -that this auditing library expects to use (returning
> +A typical implementation of this function simply returns the constant
> +.BR LAV_CURRENT ,
> +which indicates the version of
> +.I <link.h>
> +that was used to build the audit module.  If the dynamic linker does
> +not support this version of the audit interface, it will refuse to
> +activate this audit module.  If the function returns zero, the dynamic
> +linker also does not activate this audit module.
> +.PP
> +In order to enable backwards compatibility with older dynamic linkers,
> +an audit module can examine the
> +.I version
> +argument and return an earlier version than
> +.BR LAV_CURRENT ,
> +assuming the module can adjust its implementation to match the
> +requirements of the previous version of the audit interface.  The
> +.B la_version
> +function should not return the value of
>  .I version
> -is acceptable).
> -If the returned value is 0,
> -or a version that is greater than that supported by the dynamic linker,
> -then the audit library is ignored.
> +without further checks because it could correspond to an interface
> +that does not match the
> +.I <link.h>
> +definitions used to build the audit module.
>  .SS la_objsearch()
>  \&
>  .nf
> @@ -508,9 +521,10 @@ This is reportedly fixed in glibc 2.10.
>  unsigned int
>  la_version(unsigned int version)
>  {
> -    printf("la_version(): %u\en", version);
> +    printf("la_version(): version = %u; LAV_CURRENT = %u\en",
> +            version, LAV_CURRENT);
>  
> -    return version;
> +    return LAV_CURRENT;
>  }
>  
>  char *
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH] rtld-audit.7: Clarify la_version handshake

[Carlos O'Donell]

On 9/25/20 6:47 AM, Michael Kerrisk (man-pages) wrote:
> On 9/25/20 10:48 AM, Florian Weimer wrote:
>> Returning its argument without further checks is almost always
>> wrong for la_version.
>>
>> Signed-off-by: Florian Weimer <fweimer@redhat.com>
> 
> Hello Florian,
> 
> I've applied this patch locally. I'll merge into master
> in a few hours. Perhaps in the meantime there are acks/reviews
> that come in.

Just for the record. This version looks good to me and addresses
my previous comments.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
 
> Cheers,
> 
> Michael
> 
>> ---
>>  man7/rtld-audit.7 | 36 +++++++++++++++++++++++++-----------
>>  1 file changed, 25 insertions(+), 11 deletions(-)
>>
>> diff --git a/man7/rtld-audit.7 b/man7/rtld-audit.7
>> index b1b7dfebc..21cc339d0 100644
>> --- a/man7/rtld-audit.7
>> +++ b/man7/rtld-audit.7
>> @@ -70,17 +70,30 @@ the auditing library.
>>  When invoking this function, the dynamic linker passes, in
>>  .IR version ,
>>  the highest version of the auditing interface that the linker supports.
>> -If necessary, the auditing library can check that this version
>> -is sufficient for its requirements.
>>  .PP
>> -As its function result,
>> -this function should return the version of the auditing interface
>> -that this auditing library expects to use (returning
>> +A typical implementation of this function simply returns the constant
>> +.BR LAV_CURRENT ,
>> +which indicates the version of
>> +.I <link.h>
>> +that was used to build the audit module.  If the dynamic linker does
>> +not support this version of the audit interface, it will refuse to
>> +activate this audit module.  If the function returns zero, the dynamic
>> +linker also does not activate this audit module.
>> +.PP
>> +In order to enable backwards compatibility with older dynamic linkers,
>> +an audit module can examine the
>> +.I version
>> +argument and return an earlier version than
>> +.BR LAV_CURRENT ,
>> +assuming the module can adjust its implementation to match the
>> +requirements of the previous version of the audit interface.  The
>> +.B la_version
>> +function should not return the value of
>>  .I version
>> -is acceptable).
>> -If the returned value is 0,
>> -or a version that is greater than that supported by the dynamic linker,
>> -then the audit library is ignored.
>> +without further checks because it could correspond to an interface
>> +that does not match the
>> +.I <link.h>
>> +definitions used to build the audit module.
>>  .SS la_objsearch()
>>  \&
>>  .nf
>> @@ -508,9 +521,10 @@ This is reportedly fixed in glibc 2.10.
>>  unsigned int
>>  la_version(unsigned int version)
>>  {
>> -    printf("la_version(): %u\en", version);
>> +    printf("la_version(): version = %u; LAV_CURRENT = %u\en",
>> +            version, LAV_CURRENT);
>>  
>> -    return version;
>> +    return LAV_CURRENT;
>>  }
>>  
>>  char *
>>
> 
> 


-- 
Cheers,
Carlos.

Re: [PATCH] rtld-audit.7: Clarify la_version handshake

[Michael Kerrisk (man-pages)]

On Fri, 25 Sep 2020 at 18:14, Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 9/25/20 6:47 AM, Michael Kerrisk (man-pages) wrote:
> > On 9/25/20 10:48 AM, Florian Weimer wrote:
> >> Returning its argument without further checks is almost always
> >> wrong for la_version.
> >>
> >> Signed-off-by: Florian Weimer <fweimer@redhat.com>
> >
> > Hello Florian,
> >
> > I've applied this patch locally. I'll merge into master
> > in a few hours. Perhaps in the meantime there are acks/reviews
> > that come in.
>
> Just for the record. This version looks good to me and addresses
> my previous comments.
>
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>

Thanks, Carlos!

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/