Re: [RFC PATCH] powerpc/vdso: Should VDSO64 functions be flagged as functions like VDSO32 ?

[Michael Ellerman <mpe@ellerman.id.au>]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Hi Michael,
>
> Le 18/09/2024 à 04:33, Michael Ellerman a écrit :
>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>> On powerpc64 as shown below by readelf, vDSO functions symbols have
>>> type NOTYPE.
>>>
>>> $ powerpc64-linux-gnu-readelf -a arch/powerpc/kernel/vdso/vdso64.so.dbg
>>> ELF Header:
>>>    Magic:   7f 45 4c 46 02 02 01 00 00 00 00 00 00 00 00 00
>>>    Class:                             ELF64
>>>    Data:                              2's complement, big endian
>>>    Version:                           1 (current)
>>>    OS/ABI:                            UNIX - System V
>>>    ABI Version:                       0
>>>    Type:                              DYN (Shared object file)
>>>    Machine:                           PowerPC64
>>>    Version:                           0x1
>>> ...
>>>
>>> Symbol table '.dynsym' contains 12 entries:
>>>     Num:    Value          Size Type    Bind   Vis      Ndx Name
>>> ...
>>>       1: 0000000000000524    84 NOTYPE  GLOBAL DEFAULT    8 __[...]@@LINUX_2.6.15
>>> ...
>>>       4: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  ABS LINUX_2.6.15
>>>       5: 00000000000006c0    48 NOTYPE  GLOBAL DEFAULT    8 __[...]@@LINUX_2.6.15
>>>
>>> Symbol table '.symtab' contains 56 entries:
>>>     Num:    Value          Size Type    Bind   Vis      Ndx Name
>>> ...
>>>      45: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  ABS LINUX_2.6.15
>>>      46: 00000000000006c0    48 NOTYPE  GLOBAL DEFAULT    8 __kernel_getcpu
>>>      47: 0000000000000524    84 NOTYPE  GLOBAL DEFAULT    8 __kernel_clock_getres
>>>
>>> To overcome that, commit ba83b3239e65 ("selftests: vDSO: fix vDSO
>>> symbols lookup for powerpc64") was proposed to make selftests also
>>> look for NOTYPE symbols, but is it the correct fix ?
>>>
>>> VDSO32 functions are flagged as functions, why not VDSO64 functions ?
>>> Is it because VDSO functions are not traditional C functions using
>>> the standard API ?
>> 
>> Yes. There's some explanation in the original commit:
>> 
>>      Note that the symbols exposed by the vDSO aren't "normal" function symbols, apps
>>      can't be expected to link against them directly, the vDSO's are both seen
>>      as if they were linked at 0 and the symbols just contain offsets to the
>>      various functions.  This is done on purpose to avoid a relocation step
>>      (ppc64 functions normally have descriptors with abs addresses in them).
>>      When glibc uses those functions, it's expected to use it's own trampolines
>>      that know how to reach them.
>> 
>>  From https://github.com/mpe/linux-fullhistory/commit/5f2dd691b62da9d9cc54b938f8b29c22c93cb805
>> 
>> The descriptors it's talking about are the OPD function descriptors used
>> on ABI v1 (big endian).
>> 
>>> But it is exactly the same for VDSO32 functions, allthough they are
>>> flagged as functions.
>>   
>> It's not quite the same because of the function descriptors.
>> 
>> On ppc64/ABIv1 a function pointer for "F" points to an opd, which then
>> points to ".F" which has the actual text. It's the ".F" symbol that has
>> type "function".
>> 
>>> So lets flag them as functions and revert the selftest change.
>>>
>>> What's your opinion on that ?
>> 
>> I think it's fine on ppc64le, I worry slightly that it risks breaking
>> glibc or something else on big endian.
>> 
>> It is more correct for the text symbol to have type function, even if
>> there's no function descriptor for it.
>> 
>> glibc has a special case already for handling the VDSO symbols which
>> creates a fake opd pointing at the kernel symbol. So changing the VDSO
>> symbol type to function shouldn't affect that AFAICS.
>> 
>> I think the only cause of breakage would be if something is explicitly
>> looking for NOTYPE symbols, which seems unlikely, but you never know.
>> 
>> So I think we could attempt to take this change for v6.13, giving it
>> lots of time to get some test coverage in next before going to mainline.
>
> Will you take the RFC as is for 6.13 or would you like me to include the 
> above explainations and repost as non-RFC ?

If you can come up with a consolidated changelog and post a non-RFC
version that would help, thanks.

cheers