Re: [PATCH] powerpc: Restore NULL check on _rtld_global_ro in INIT_ARCH [BZ #34503]

On 10/08/26 07:41, Michael Pfeifroth wrote:
> Sorry, v1 was mangled by my mail client (tabs converted to spaces on
> paste from X11 primary selection).  Resending correctly.  No changes
> from v1 otherwise.
> 
> -- >8 --
> Commit 21841f0d562f ("PowerPC: Influence cpu/arch hwcap features via
> GLIBC_TUNABLES") changed the INIT_ARCH() macro used by powerpc32/power4
> and (via a one-line include) powerpc64 multiarch IFUNC resolvers to
> read hwcap and hwcap2 through a direct
> 
>     &GLRO(dl_powerpc_cpu_features)
> 
> reference, instead of the previous __GLRO() wrapper.  The __GLRO() macro
> performs a volatile NULL check on _rtld_global_ro, which matters because
> IFUNC resolvers can run before _rtld_global_ro has been relocated for the
> current library.
> 
> This regression triggers when a shared library's IFUNC symbol from libm
> is resolved via BIND_NOW (full RELRO) before libm's own GOT is relocated:
> the resolver's INIT_ARCH() then dereferences a NULL _rtld_global_ro and
> segfaults at the hwcap load.  The concrete failure seen was rsyslogd
> crashing on startup on powerpc64 (e5500, BE) with
> 
>     rsyslogd -> librsyslog -> libfastjson -> modf() IFUNC in libm
> 
> when libfastjson lacked a DT_NEEDED on libm, so libm was relocated after
> libfastjson's IFUNC resolvers ran.
> 
> Restore the __GLRO()-based access for both hwcap and hwcap2, matching
> the pre-2.41 behaviour and how use_cached_memopt is already read in the
> same macro.  This is a no-op once _rtld_global_ro is fully initialised
> and simply reinstates the early-startup NULL guard.
> 
> Signed-off-by: Michael Pfeifroth <[email protected]>

The patch looks good, but maybe it would be good to add a regression testcase
for that. Something like:

$ cat << EOF > lib.c
extern double modf (double, double *);
double (*volatile ifunc_ptr) (double, double *) = modf;
EOF

$ cat << EOF > main.c
#include <stdio.h>
extern double (*volatile ifunc_ptr) (double, double *);
int main (int argc, char *argv[])
{
  double ip;
  double frac = ifunc_ptr (2.5, &ip);
  printf ("modf(2.5) = %g + %g (expect 0.5 + 2)\n", frac, ip);
  return !(frac == 0.5 && ip == 2.0);
}
EOF

$ gcc -fpic -shared -Wl,-z,now -Wl,-z,undefs -Wl,-soname,lib.so -o lib.so lib.c

$ gcc -Wall -Wl,--no-as-needed -lm ./lib.so '-Wl,-rpath,$ORIGIN' -o main main.c

This needs to be ppc64 only for now, I think ppc32 has a different issues that
prevents to use this even with this fix.

> ---
>  sysdeps/powerpc/powerpc32/power4/multiarch/init-arch.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/sysdeps/powerpc/powerpc32/power4/multiarch/init-arch.h b/sysdeps/powerpc/powerpc32/power4/multiarch/init-arch.h
> index 9aab67a5..e681b452 100644
> --- a/sysdeps/powerpc/powerpc32/power4/multiarch/init-arch.h
> +++ b/sysdeps/powerpc/powerpc32/power4/multiarch/init-arch.h
> @@ -36,9 +36,9 @@
>  /* Get the hardware information post the tunables set, the macro checks
>     it and fills the previous ones.  */
>  #define INIT_ARCH() \
> -  const struct cpu_features *features = &GLRO(dl_powerpc_cpu_features);	\
> -  unsigned long int hwcap = features->hwcap;				\
> -  unsigned long int __attribute__((unused)) hwcap2 = features->hwcap2; \
> +  unsigned long int hwcap = __GLRO(dl_powerpc_cpu_features.hwcap);	\
> +  unsigned long int __attribute__((unused)) hwcap2 =			\
> +    __GLRO(dl_powerpc_cpu_features.hwcap2);				\
>    bool __attribute__((unused)) use_cached_memopt =		\
>      __GLRO(dl_powerpc_cpu_features.use_cached_memopt);		\
>    if (hwcap & PPC_FEATURE_ARCH_2_06)				\