Re: [PATCH v4 1/3] riscv: Add Linux hwprobe syscall support

[Evan Green <evan@rivosinc.com>]

On Mon, Jul 10, 2023 at 2:17 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Evan Green:
>
> > On Fri, Jul 7, 2023 at 1:16 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * Evan Green:
> >>
> >> > Add awareness and a thin wrapper function around a new Linux system call
> >> > that allows callers to get architecture and microarchitecture
> >> > information about the CPUs from the kernel. This can be used to
> >> > do things like dynamically choose a memcpy implementation.
> >>
> >> I missed before that you intend this for use in IFUNC resolvers, or at
> >> the very least I think I forgot to raise this caveat.
> >>
> >> RISC-V is not a HIDDEN_VAR_NEEDS_DYNAMIC_RELOC target, so this is not
> >> completely impossible, but in general, extern function calls in IFUNC
> >> resolvers tend to not work well.  The issue is that the GOT pointer for
> >> a function like __riscv_hwprobe may not have been set up when the
> >> dynamic linker invokes the IFUNC resolver.  There are several ways to
> >> solve this.  You could pass the function pointer to the IFUNC resolver
> >> (which may require a marker symbol and GCC changes).  Or you could a
> >> hidden wrapper function to libc_nonshared.a that checks if the function
> >> pointer to the libc implementation has been set up by a relocation and
> >> uses that, and falls back to a direct system call otherwise.
> >
> > That makes sense, but then I'm confused about how it's been working in
> > my testing. What experiment should I try to see this problem in
> > action? An early constructor maybe?
>
> This script should reproduce it:
>
> cat >libifunc.c <<'EOF'
> #include <dlfcn.h>
>
> typedef void *(*malloc_fptr) (size_t);
>
> static malloc_fptr
> malloc_address (void)
> {
>   malloc_fptr result = dlsym (RTLD_NEXT, "malloc");
>   asm ("");                     /* Prevent tail call.  */
>   return result;
> }
>
>
> static void *
> malloc_impl_indirect (size_t size)
> {
>   return  malloc_address () (size);
> }
>
> static void *
> malloc_resolve (void)
> {
> #ifdef DIRECT
>   return malloc_address ();
> #else
>   return &malloc_impl_indirect;
> #endif
> }
>
> void *malloc (size_t) __attribute__ ((ifunc ("malloc_resolve")));
> EOF
> gcc -DDIRECT -O2 -g -Wl,-z,now -shared -fPIC -o libifunc.so libifunc.c
> LD_PRELOAD=./libifunc.so /bin/true --help
>
> The problem here is that dlsym is called during relocation, and that
> function pointer has not been set up yet.
>
> It shouldn't be too hard to come up with a variant that uses
> __riscv_hwprobe instead.
>
> > Could I alternatively convert the implementation of the external
> > function into one that calls an internal helper, and then just call
> > the internal helper from the ifunc resolver (oh, I've reinvented a
> > protected symbol)? Or is that a violation of some rule?
>
> It depends on where the internal helper is located.  The issue manifests
> in user-compiled code, not directly in glibc, so there is no straight
> fix for this in glibc.
>
> > Are there any examples of that last suggestion I can refer to?
> > Specifically the part about checking if the symbol has been relocated
> > yet or not.
>
> You can load the symbol value using assembler code, so that you can
> check if it is null, or perhaps you can make it weak (so that GCC treats
> NULL as a valid value).
>

Thank you for the example, that helped a lot. I had mistakenly thought
you were pointing to a case where my memcpy ifunc selector broke down,
but now I realize you're talking about future ifunc selectors that
exist in applications and other libraries.

I've got something now that creates a weak alias,
__riscv_hwprobe_weak, as well as a statically compiled
__riscv_hwprobe_early() that either routes through the weak function
or makes the syscall directly as you suggested. I was able to adapt
your example to verify it fixes the crash. I'll send that out shortly,
included in a respin of this series.

-Evan