[PATCH v3] RISC-V: Fix IFUNC resolver cannot access gp pointer

[Yangyu Chen]

On 1/8/25 03:12, Palmer Dabbelt wrote:
> On Tue, 07 Jan 2025 09:00:10 PST (-0800), jrtc27@jrtc27.com wrote:
>> On 7 Jan 2025, at 16:38, Florian Weimer <fweimer@redhat.com> wrote:
>>>
>>> * Palmer Dabbelt:
>>>
>>>> On Mon, 06 Jan 2025 18:32:54 PST (-0800), cyy@cyyself.name wrote:
>>>>> Ping again. It has already been 1 month after v3 patch was submitted.
>>>>>
>>>>> I think this patch is critical for RISC-V GCC Function multi- 
>>>>> versioning
>>>>> to work correctly, as GCC 15, which has this feature, is going to be
>>>>> released.
>>>>
>>>> IIUC the conclusion here was to avoid global symbol resolution in
>>>> IFUNCs and instead use the pre-resolved function hwprobe VDSO function
>>>> pointer passed in via a register.  There was a long discussion on this
>>>> during the hwprobe/IFUNC support patches, and IIRC one during the FMV
>>>> patches -- though I remember accepting those and now I'm not sure why,
>>>> so maybe I'm just forgetting something here?
>>>
>>> As far as I understand it, at least in some cases, the gp register needs
>>> to be set up for the main program even for local static/hidden symbol
>>> access.  Unfortunately, the ABI does not provide a completely reliable
>>> way for the dynamic linker to discover the gp value the main program
>>> requires, which leads to the code we have now.  I think we should have
>>> something that is more declarative than running a piece of assembler
>>> code at the right time (the existing code runs too late, after IFUNC
>>> resolver), and which does not depend on dynamic symbol resolution.
>>
>> And, notably, source code that makes no use of global variables can
>> still end up implicitly using them, e.g. for switch statements, which
>> you could imagine being used for hwprobe-using code in the case of keys
>> that have enum values, like misaligned scalar perf. Both GCC and Clang
>> optimise to using a lookup table for switch statements over an enum
>> with that many cases, which means a global access that could become
>> GP-relative with the right (wrong?) linker and memory layout. See
>> https://godbolt.org/z/KEbof5z7h for a minimal example of that, which is
>> completely valid and sensible (modulo bugs from lack of testing) code
>> that you can write today with hwprobe and that is broken if the lookup
>> table is laid out by GNU ld to be close enough to __global_pointer$.
> 
> Here's the code, in case the godbolt short link goes away:
> 
>     typedef unsigned long size_t;
>     #define NULL (void *)0
>     struct riscv_hwprobe {
>         signed long long int key;
>         unsigned long long int value;
>     };
>     typedef int (*__riscv_hwprobe_t) (struct riscv_hwprobe *__pairs, 
> size_t __pair_count,
>                       size_t __cpu_count, unsigned long int *__cpus,
>                       unsigned int __flags);
>     #define RISCV_HWPROBE_KEY_MISALIGNED_SCALAR_PERF    9
>     #define        RISCV_HWPROBE_MISALIGNED_SCALAR_UNKNOWN        0
>     #define        RISCV_HWPROBE_MISALIGNED_SCALAR_EMULATED    1
>     #define        RISCV_HWPROBE_MISALIGNED_SCALAR_SLOW        2
>     #define        RISCV_HWPROBE_MISALIGNED_SCALAR_FAST        3
>     #define        RISCV_HWPROBE_MISALIGNED_SCALAR_UNSUPPORTED    4
>     #define __used __attribute__((used))
>     #define __unused __attribute__((unused))
>     #define    DEFINE_UIFUNC(qual, ret_type, name, args)            \
>         static ret_type (*name##_resolver(unsigned long, 
> __riscv_hwprobe_t,    \
>         unsigned long, unsigned long, unsigned long, unsigned long,    \
>         unsigned long, unsigned long))args __used;            \
>         qual ret_type name args __attribute__((ifunc(#name 
> "_resolver")));    \
>         static ret_type (*name##_resolver(unsigned long hwcap 
> __unused,    \
>         __riscv_hwprobe_t hwprobe __unused, unsigned long _arg3 
> __unused,    \
>         unsigned long _arg4 __unused, unsigned long _arg5 __unused,    \
>         unsigned long _arg6 __unused, unsigned long _arg7 __unused,    \
>         unsigned long _arg8 __unused))args
>     void foo_unknown(void) {}
>     void foo_emulated(void) {}
>     void foo_slow(void) {}
>     void foo_fast(void) {}
>     void foo_unsupported(void) {}
>     DEFINE_UIFUNC(, void, foo, (void))
>     {
>         struct riscv_hwprobe pair;
>         pair.key = RISCV_HWPROBE_KEY_MISALIGNED_SCALAR_PERF;
>         if (hwprobe(&pair, 1, 0, NULL, 0) != 0)
>             goto unknown;
>         switch (pair.value) {
>         unknown:
>         default:
>         case RISCV_HWPROBE_MISALIGNED_SCALAR_UNKNOWN:
>             return foo_unknown;        case 
> RISCV_HWPROBE_MISALIGNED_SCALAR_EMULATED:
>             return foo_emulated;
>         case RISCV_HWPROBE_MISALIGNED_SCALAR_SLOW:
>             return foo_slow;
>         case RISCV_HWPROBE_MISALIGNED_SCALAR_FAST:
>             return foo_fast;
>         case RISCV_HWPROBE_MISALIGNED_SCALAR_UNSUPPORTED:
>             return foo_unsupported;
>         }
>     }
> 
> At least in GCC the docs for `-fjump-table` say to turn it off if you're in
> code that can't handle getting symbols references added, so if the rules 
> are to
> avoid resolving symbols in IFUNC resolvers then that's just user error 
> (though
> kind of a clunky one).  So I think GP is really just a red herring, here --
> sure it's one way this can break, but we went through months of other 
> reasons
> this can break when we decided not to allow them.
> 
> That said, I think there's an even bigger issue here: if we're expecting 
> users
> to return function pointers from the IFUNC resolver, then how are they 
> supposed
> to get those function pointers pre-relocation?  Even if it was easy to 
> get the
> compiler to only use PC-relative references, those would still just be 
> wrong.

I think this is a feature of IFUNC that should not reference any symbol 
that requires relocation. Luckily, the current C-API design and GCC 
implementation of FMV have already prevented this. Also, any 
user-provided IFUNC resolver should be aware of this. This is not an 
architecture-specific issue related to RISC-V.

Thanks,
Yangyu Chen

> 
> So unless I'm just lost here, I think we've got something else wrong?
> 
>> Jess