public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [PATCH v2] RISC-V: Fix IFUNC resolver cannot access gp pointer
@ 2024-10-15 17:01 Yangyu Chen
  2024-10-30  2:58 ` Yangyu Chen
  0 siblings, 1 reply; 16+ messages in thread
From: Yangyu Chen @ 2024-10-15 17:01 UTC (permalink / raw)
  To: libc-alpha
  Cc: Palmer Dabbelt, Kito Cheng, Vincent Chen, Andreas Schwab, Yangyu Chen

In some cases, an IFUNC resolver may need to access the gp pointer to
resolve the function address. Such an object may have l_relocated == 0.
In this case, the GP register will not be set up. Thus, the IFUNC
resolver cannot access the gp pointer. This patch fixes this issue by
relaxing the check of l_relocated in elf_machine_runtime_setup.

As for the original Bug 31317, since the static-linked executable has
already set up the gp pointer, we don't need to execute the code to set
up the gp pointer again. I have also reproduced and checked Bug 31317,
this patch can fix the issue.

Closes: BZ #32269
Fixes: 96d1b9ac23 ("RISC-V: Fix the static-PIE non-relocated object check")
Signed-off-by: Yangyu Chen <cyy@cyyself.name>
---
 sysdeps/riscv/dl-machine.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sysdeps/riscv/dl-machine.h b/sysdeps/riscv/dl-machine.h
index b2f28697f7..10a36d6701 100644
--- a/sysdeps/riscv/dl-machine.h
+++ b/sysdeps/riscv/dl-machine.h
@@ -348,7 +348,8 @@ elf_machine_runtime_setup (struct link_map *l, struct r_scope_elem *scope[],
       gotplt[1] = (ElfW(Addr)) l;
     }
 
-  if (l->l_type == lt_executable && l->l_relocated)
+#ifdef SHARED
+  if (l->l_type == lt_executable)
     {
       /* The __global_pointer$ may not be defined by the linker if the
 	 $gp register does not be used to access the global variable
@@ -368,6 +369,7 @@ elf_machine_runtime_setup (struct link_map *l, struct r_scope_elem *scope[],
           : "r" (ref->st_value)
         );
     }
+#endif
 #endif
   return lazy;
 }
-- 
2.45.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] RISC-V: Fix IFUNC resolver cannot access gp pointer
  2024-10-15 17:01 [PATCH v2] RISC-V: Fix IFUNC resolver cannot access gp pointer Yangyu Chen
@ 2024-10-30  2:58 ` Yangyu Chen
  2024-10-30  3:10   ` Palmer Dabbelt
  0 siblings, 1 reply; 16+ messages in thread
From: Yangyu Chen @ 2024-10-30  2:58 UTC (permalink / raw)
  To: libc-alpha; +Cc: Palmer Dabbelt, Kito Cheng, Vincent Chen, Andreas Schwab

Friendly ping.

FYI, this bug can be reproduced through the steps outlined in the BZ #32269.

Link: https://sourceware.org/bugzilla/show_bug.cgi?id=32269

On 10/16/24 01:01, Yangyu Chen wrote:
> In some cases, an IFUNC resolver may need to access the gp pointer to
> resolve the function address. Such an object may have l_relocated == 0.
> In this case, the GP register will not be set up. Thus, the IFUNC
> resolver cannot access the gp pointer. This patch fixes this issue by
> relaxing the check of l_relocated in elf_machine_runtime_setup.
> 
> As for the original Bug 31317, since the static-linked executable has
> already set up the gp pointer, we don't need to execute the code to set
> up the gp pointer again. I have also reproduced and checked Bug 31317,
> this patch can fix the issue.
> 
> Closes: BZ #32269
> Fixes: 96d1b9ac23 ("RISC-V: Fix the static-PIE non-relocated object check")
> Signed-off-by: Yangyu Chen <cyy@cyyself.name>
> ---
>   sysdeps/riscv/dl-machine.h | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/sysdeps/riscv/dl-machine.h b/sysdeps/riscv/dl-machine.h
> index b2f28697f7..10a36d6701 100644
> --- a/sysdeps/riscv/dl-machine.h
> +++ b/sysdeps/riscv/dl-machine.h
> @@ -348,7 +348,8 @@ elf_machine_runtime_setup (struct link_map *l, struct r_scope_elem *scope[],
>         gotplt[1] = (ElfW(Addr)) l;
>       }
>   
> -  if (l->l_type == lt_executable && l->l_relocated)
> +#ifdef SHARED
> +  if (l->l_type == lt_executable)
>       {
>         /* The __global_pointer$ may not be defined by the linker if the
>   	 $gp register does not be used to access the global variable
> @@ -368,6 +369,7 @@ elf_machine_runtime_setup (struct link_map *l, struct r_scope_elem *scope[],
>             : "r" (ref->st_value)
>           );
>       }
> +#endif
>   #endif
>     return lazy;
>   }



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] RISC-V: Fix IFUNC resolver cannot access gp pointer
  2024-10-30  2:58 ` Yangyu Chen
@ 2024-10-30  3:10   ` Palmer Dabbelt
  2024-10-30  6:50     ` Yangyu Chen
  2024-11-13 10:29     ` Yangyu Chen
  0 siblings, 2 replies; 16+ messages in thread
From: Palmer Dabbelt @ 2024-10-30  3:10 UTC (permalink / raw)
  To: cyy; +Cc: libc-alpha, kito.cheng, vincent.chen, schwab

On Tue, 29 Oct 2024 19:58:14 PDT (-0700), cyy@cyyself.name wrote:
> Friendly ping.
>
> FYI, this bug can be reproduced through the steps outlined in the BZ #32269.
>
> Link: https://sourceware.org/bugzilla/show_bug.cgi?id=32269
>
> On 10/16/24 01:01, Yangyu Chen wrote:
>> In some cases, an IFUNC resolver may need to access the gp pointer to
>> resolve the function address. Such an object may have l_relocated == 0.
>> In this case, the GP register will not be set up. Thus, the IFUNC
>> resolver cannot access the gp pointer. This patch fixes this issue by
>> relaxing the check of l_relocated in elf_machine_runtime_setup.
>>
>> As for the original Bug 31317, since the static-linked executable has
>> already set up the gp pointer, we don't need to execute the code to set
>> up the gp pointer again. I have also reproduced and checked Bug 31317,
>> this patch can fix the issue.

We had a pretty similar issue with the hwprobe IFUNC stuff, and IIRC the 
result there was that it's just not safe to look up global symbols from 
an IFUNC.

>> Closes: BZ #32269
>> Fixes: 96d1b9ac23 ("RISC-V: Fix the static-PIE non-relocated object check")
>> Signed-off-by: Yangyu Chen <cyy@cyyself.name>
>> ---
>>   sysdeps/riscv/dl-machine.h | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/sysdeps/riscv/dl-machine.h b/sysdeps/riscv/dl-machine.h
>> index b2f28697f7..10a36d6701 100644
>> --- a/sysdeps/riscv/dl-machine.h
>> +++ b/sysdeps/riscv/dl-machine.h
>> @@ -348,7 +348,8 @@ elf_machine_runtime_setup (struct link_map *l, struct r_scope_elem *scope[],
>>         gotplt[1] = (ElfW(Addr)) l;
>>       }
>>
>> -  if (l->l_type == lt_executable && l->l_relocated)
>> +#ifdef SHARED
>> +  if (l->l_type == lt_executable)
>>       {
>>         /* The __global_pointer$ may not be defined by the linker if the
>>   	 $gp register does not be used to access the global variable
>> @@ -368,6 +369,7 @@ elf_machine_runtime_setup (struct link_map *l, struct r_scope_elem *scope[],
>>             : "r" (ref->st_value)
>>           );
>>       }
>> +#endif
>>   #endif
>>     return lazy;
>>   }

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] RISC-V: Fix IFUNC resolver cannot access gp pointer
  2024-10-30  3:10   ` Palmer Dabbelt
@ 2024-10-30  6:50     ` Yangyu Chen
  2024-10-30  7:36       ` Kito Cheng
  2024-11-13 10:29     ` Yangyu Chen
  1 sibling, 1 reply; 16+ messages in thread
From: Yangyu Chen @ 2024-10-30  6:50 UTC (permalink / raw)
  To: Palmer Dabbelt; +Cc: libc-alpha, kito.cheng, vincent.chen, schwab



> On Oct 30, 2024, at 11:10, Palmer Dabbelt <palmer@rivosinc.com> wrote:
> 
> On Tue, 29 Oct 2024 19:58:14 PDT (-0700), cyy@cyyself.name wrote:
>> Friendly ping.
>> 
>> FYI, this bug can be reproduced through the steps outlined in the BZ #32269.
>> 
>> Link: https://sourceware.org/bugzilla/show_bug.cgi?id=32269
>> 
>> On 10/16/24 01:01, Yangyu Chen wrote:
>>> In some cases, an IFUNC resolver may need to access the gp pointer to
>>> resolve the function address. Such an object may have l_relocated == 0.
>>> In this case, the GP register will not be set up. Thus, the IFUNC
>>> resolver cannot access the gp pointer. This patch fixes this issue by
>>> relaxing the check of l_relocated in elf_machine_runtime_setup.
>>> 
>>> As for the original Bug 31317, since the static-linked executable has
>>> already set up the gp pointer, we don't need to execute the code to set
>>> up the gp pointer again. I have also reproduced and checked Bug 31317,
>>> this patch can fix the issue.
> 
> We had a pretty similar issue with the hwprobe IFUNC stuff, and IIRC the result there was that it's just not safe to look up global symbols from an IFUNC.
> 

However, using global variables in IFUNC is unavoidable when using
the data structure __riscv_feature_bits from RISC-V C-API [1], which
stores a global variable and is initialized by a function in libgcc
(GCC) [2] or compiler-rt (LLVM) [3]. I encountered this bug while
implementing target_clones and target_version for RISC-V GCC [4].

I have no better idea to solve this problem. Perhaps we should add
some new restrictions to the ABI?

[1] https://github.com/riscv-non-isa/riscv-c-api-doc/blob/main/src/c-api.adoc#extension-bitmask
[2] https://github.com/gcc-mirror/gcc/blob/master/libgcc/config/riscv/feature_bits.c
[3] https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/builtins/cpu_model/riscv.c
[4] https://patchwork.sourceware.org/project/gcc/list/?series=39863&state=*

>>> Closes: BZ #32269
>>> Fixes: 96d1b9ac23 ("RISC-V: Fix the static-PIE non-relocated object check")
>>> Signed-off-by: Yangyu Chen <cyy@cyyself.name>
>>> ---
>>>  sysdeps/riscv/dl-machine.h | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/sysdeps/riscv/dl-machine.h b/sysdeps/riscv/dl-machine.h
>>> index b2f28697f7..10a36d6701 100644
>>> --- a/sysdeps/riscv/dl-machine.h
>>> +++ b/sysdeps/riscv/dl-machine.h
>>> @@ -348,7 +348,8 @@ elf_machine_runtime_setup (struct link_map *l, struct r_scope_elem *scope[],
>>>        gotplt[1] = (ElfW(Addr)) l;
>>>      }
>>> 
>>> -  if (l->l_type == lt_executable && l->l_relocated)
>>> +#ifdef SHARED
>>> +  if (l->l_type == lt_executable)
>>>      {
>>>        /* The __global_pointer$ may not be defined by the linker if the
>>>    $gp register does not be used to access the global variable
>>> @@ -368,6 +369,7 @@ elf_machine_runtime_setup (struct link_map *l, struct r_scope_elem *scope[],
>>>            : "r" (ref->st_value)
>>>          );
>>>      }
>>> +#endif
>>>  #endif
>>>    return lazy;
>>>  }



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] RISC-V: Fix IFUNC resolver cannot access gp pointer
  2024-10-30  6:50     ` Yangyu Chen
@ 2024-10-30  7:36       ` Kito Cheng
  2024-10-30  9:22         ` Florian Weimer
  0 siblings, 1 reply; 16+ messages in thread
From: Kito Cheng @ 2024-10-30  7:36 UTC (permalink / raw)
  To: Yangyu Chen; +Cc: Palmer Dabbelt, libc-alpha, vincent.chen, schwab

Hi Palmer:

> >>> In some cases, an IFUNC resolver may need to access the gp pointer to
> >>> resolve the function address. Such an object may have l_relocated == 0.
> >>> In this case, the GP register will not be set up. Thus, the IFUNC
> >>> resolver cannot access the gp pointer. This patch fixes this issue by
> >>> relaxing the check of l_relocated in elf_machine_runtime_setup.
> >>>
> >>> As for the original Bug 31317, since the static-linked executable has
> >>> already set up the gp pointer, we don't need to execute the code to set
> >>> up the gp pointer again. I have also reproduced and checked Bug 31317,
> >>> this patch can fix the issue.
> >
> > We had a pretty similar issue with the hwprobe IFUNC stuff, and IIRC the result there was that it's just not safe to look up global symbols from an IFUNC.

Do you remember if there are any other issues that need to be resolved
other than the GP issue?

Or do you think it's not the right solution for the GP issue or it may
have potential risk?

> >
>
> However, using global variables in IFUNC is unavoidable when using
> the data structure __riscv_feature_bits from RISC-V C-API [1], which
> stores a global variable and is initialized by a function in libgcc
> (GCC) [2] or compiler-rt (LLVM) [3]. I encountered this bug while
> implementing target_clones and target_version for RISC-V GCC [4].
>
> I have no better idea to solve this problem. Perhaps we should add
> some new restrictions to the ABI?
>
> [1] https://github.com/riscv-non-isa/riscv-c-api-doc/blob/main/src/c-api.adoc#extension-bitmask
> [2] https://github.com/gcc-mirror/gcc/blob/master/libgcc/config/riscv/feature_bits.c
> [3] https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/builtins/cpu_model/riscv.c
> [4] https://patchwork.sourceware.org/project/gcc/list/?series=39863&state=*
>
> >>> Closes: BZ #32269
> >>> Fixes: 96d1b9ac23 ("RISC-V: Fix the static-PIE non-relocated object check")
> >>> Signed-off-by: Yangyu Chen <cyy@cyyself.name>
> >>> ---
> >>>  sysdeps/riscv/dl-machine.h | 4 +++-
> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/sysdeps/riscv/dl-machine.h b/sysdeps/riscv/dl-machine.h
> >>> index b2f28697f7..10a36d6701 100644
> >>> --- a/sysdeps/riscv/dl-machine.h
> >>> +++ b/sysdeps/riscv/dl-machine.h
> >>> @@ -348,7 +348,8 @@ elf_machine_runtime_setup (struct link_map *l, struct r_scope_elem *scope[],
> >>>        gotplt[1] = (ElfW(Addr)) l;
> >>>      }
> >>>
> >>> -  if (l->l_type == lt_executable && l->l_relocated)
> >>> +#ifdef SHARED
> >>> +  if (l->l_type == lt_executable)
> >>>      {
> >>>        /* The __global_pointer$ may not be defined by the linker if the
> >>>    $gp register does not be used to access the global variable
> >>> @@ -368,6 +369,7 @@ elf_machine_runtime_setup (struct link_map *l, struct r_scope_elem *scope[],
> >>>            : "r" (ref->st_value)
> >>>          );
> >>>      }
> >>> +#endif
> >>>  #endif
> >>>    return lazy;
> >>>  }
>
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] RISC-V: Fix IFUNC resolver cannot access gp pointer
  2024-10-30  7:36       ` Kito Cheng
@ 2024-10-30  9:22         ` Florian Weimer
  2024-10-30 10:26           ` Kito Cheng
  0 siblings, 1 reply; 16+ messages in thread
From: Florian Weimer @ 2024-10-30  9:22 UTC (permalink / raw)
  To: Kito Cheng; +Cc: Yangyu Chen, Palmer Dabbelt, libc-alpha, vincent.chen, schwab

* Kito Cheng:

> Do you remember if there are any other issues that need to be resolved
> other than the GP issue?

In certain cases, IFUNC resolvers can be invoked when the object that
provides has not been relocated yet.  That's impossible to address
from within the IFUNC resolver.  (For the GP issue, the IFUNC resolver
could just assume that GP isn't defined, I think.)

Thanks,
Florian


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] RISC-V: Fix IFUNC resolver cannot access gp pointer
  2024-10-30  9:22         ` Florian Weimer
@ 2024-10-30 10:26           ` Kito Cheng
  2024-10-30 10:42             ` Florian Weimer
  0 siblings, 1 reply; 16+ messages in thread
From: Kito Cheng @ 2024-10-30 10:26 UTC (permalink / raw)
  To: Florian Weimer
  Cc: Yangyu Chen, Palmer Dabbelt, libc-alpha, vincent.chen, schwab

HI Florian:

On Wed, Oct 30, 2024 at 5:22 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Kito Cheng:
>
> > Do you remember if there are any other issues that need to be resolved
> > other than the GP issue?
>
> In certain cases, IFUNC resolvers can be invoked when the object that
> provides has not been relocated yet.  That's impossible to address
> from within the IFUNC resolver.  (For the GP issue, the IFUNC resolver
> could just assume that GP isn't defined, I think.)

Hmmmm, I am a little surprised that since we are kinda learning the
function multi-version (FMV) from AArch64[1] and x86[2], they both
will reference global variables during the FMV IFUNC resolver, or is
it just bad design from your point of view?

Thanks :)

[1] https://github.com/gcc-mirror/gcc/blob/master/libgcc/config/aarch64/cpuinfo.c#L47
[2] https://github.com/gcc-mirror/gcc/blob/master/libgcc/config/i386/cpuinfo.c#L50C14-L50C29

>
> Thanks,
> Florian
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] RISC-V: Fix IFUNC resolver cannot access gp pointer
  2024-10-30 10:26           ` Kito Cheng
@ 2024-10-30 10:42             ` Florian Weimer
  2024-10-30 10:48               ` Kito Cheng
  0 siblings, 1 reply; 16+ messages in thread
From: Florian Weimer @ 2024-10-30 10:42 UTC (permalink / raw)
  To: Kito Cheng; +Cc: Yangyu Chen, Palmer Dabbelt, libc-alpha, vincent.chen, schwab

* Kito Cheng:

> HI Florian:
>
> On Wed, Oct 30, 2024 at 5:22 PM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * Kito Cheng:
>>
>> > Do you remember if there are any other issues that need to be resolved
>> > other than the GP issue?
>>
>> In certain cases, IFUNC resolvers can be invoked when the object that
>> provides has not been relocated yet.  That's impossible to address
>> from within the IFUNC resolver.  (For the GP issue, the IFUNC resolver
>> could just assume that GP isn't defined, I think.)
>
> Hmmmm, I am a little surprised that since we are kinda learning the
> function multi-version (FMV) from AArch64[1] and x86[2], they both
> will reference global variables during the FMV IFUNC resolver, or is
> it just bad design from your point of view?

The default code model for AArch64 and x86-64 uses more or less direct
PC-relative addressing for local data references.  For this reason, that
data is in libgcc.a, with hidden visibility.  On x86-64, there is a
large code model that requires a register like GP, but every function is
required to set it up if it needs it (including PLT stubs, and %r11 is
reserved for that purposes).

The use of the GP register makes RISC-V a HIDDEN_VAR_NEEDS_DYNAMIC_RELOC
target in glibc's terms, and those can't use any data references in
IFUNC resolvers (unless you ban cyclic references, LD_PRELOAD etc.).

Thanks,
Florian


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] RISC-V: Fix IFUNC resolver cannot access gp pointer
  2024-10-30 10:42             ` Florian Weimer
@ 2024-10-30 10:48               ` Kito Cheng
  2024-10-30 11:09                 ` Florian Weimer
  0 siblings, 1 reply; 16+ messages in thread
From: Kito Cheng @ 2024-10-30 10:48 UTC (permalink / raw)
  To: Florian Weimer
  Cc: Yangyu Chen, Palmer Dabbelt, libc-alpha, vincent.chen, schwab

Hi Florian:

Thanks for your explanation! one more question around the
HIDDEN_VAR_NEEDS_DYNAMIC_RELOC,
RISC-V don't set that to 1 but seems to work well so far...?
Is it because the GP register is limited to executable so that we are
safe without that?

On Wed, Oct 30, 2024 at 6:42 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Kito Cheng:
>
> > HI Florian:
> >
> > On Wed, Oct 30, 2024 at 5:22 PM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * Kito Cheng:
> >>
> >> > Do you remember if there are any other issues that need to be resolved
> >> > other than the GP issue?
> >>
> >> In certain cases, IFUNC resolvers can be invoked when the object that
> >> provides has not been relocated yet.  That's impossible to address
> >> from within the IFUNC resolver.  (For the GP issue, the IFUNC resolver
> >> could just assume that GP isn't defined, I think.)
> >
> > Hmmmm, I am a little surprised that since we are kinda learning the
> > function multi-version (FMV) from AArch64[1] and x86[2], they both
> > will reference global variables during the FMV IFUNC resolver, or is
> > it just bad design from your point of view?
>
> The default code model for AArch64 and x86-64 uses more or less direct
> PC-relative addressing for local data references.  For this reason, that
> data is in libgcc.a, with hidden visibility.  On x86-64, there is a
> large code model that requires a register like GP, but every function is
> required to set it up if it needs it (including PLT stubs, and %r11 is
> reserved for that purposes).
>
> The use of the GP register makes RISC-V a HIDDEN_VAR_NEEDS_DYNAMIC_RELOC
> target in glibc's terms, and those can't use any data references in
> IFUNC resolvers (unless you ban cyclic references, LD_PRELOAD etc.).
>
> Thanks,
> Florian
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] RISC-V: Fix IFUNC resolver cannot access gp pointer
  2024-10-30 10:48               ` Kito Cheng
@ 2024-10-30 11:09                 ` Florian Weimer
  2024-10-30 18:28                   ` Yangyu Chen
  0 siblings, 1 reply; 16+ messages in thread
From: Florian Weimer @ 2024-10-30 11:09 UTC (permalink / raw)
  To: Kito Cheng; +Cc: Yangyu Chen, Palmer Dabbelt, libc-alpha, vincent.chen, schwab

* Kito Cheng:

> Hi Florian:
>
> Thanks for your explanation! one more question around the
> HIDDEN_VAR_NEEDS_DYNAMIC_RELOC,
> RISC-V don't set that to 1 but seems to work well so far...?
> Is it because the GP register is limited to executable so that we are
> safe without that?

Yes, exactly.  The dynamic linker doesn't use the GP register for its
own purposes, and it doesn't impact code generation for the dynamic
linker itself.

However, the way GP setup is currently handled is highly problematic
because the ABI does not give us a proper way to set it up correctly.
The use of a dynamic symbol is too fragile and seems to have been added
as an afterthought.  This should be a dynamic tag in the main
executable.  If we had that, we could set GP in all relevant calls to
application code (of course this assumes that applications do not use GP
as a global register variable for a different purpose, but hopefully
that's not the case).

Thanks,
Florian


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] RISC-V: Fix IFUNC resolver cannot access gp pointer
  2024-10-30 11:09                 ` Florian Weimer
@ 2024-10-30 18:28                   ` Yangyu Chen
  2024-10-31 10:07                     ` Kito Cheng
  2024-10-31 10:32                     ` Florian Weimer
  0 siblings, 2 replies; 16+ messages in thread
From: Yangyu Chen @ 2024-10-30 18:28 UTC (permalink / raw)
  To: Florian Weimer
  Cc: Kito Cheng, Palmer Dabbelt, libc-alpha, vincent.chen, schwab



> On Oct 30, 2024, at 19:09, Florian Weimer <fweimer@redhat.com> wrote:
> 
> * Kito Cheng:
> 
>> Hi Florian:
>> 
>> Thanks for your explanation! one more question around the
>> HIDDEN_VAR_NEEDS_DYNAMIC_RELOC,
>> RISC-V don't set that to 1 but seems to work well so far...?
>> Is it because the GP register is limited to executable so that we are
>> safe without that?
> 
> Yes, exactly.  The dynamic linker doesn't use the GP register for its
> own purposes, and it doesn't impact code generation for the dynamic
> linker itself.
> 
> However, the way GP setup is currently handled is highly problematic
> because the ABI does not give us a proper way to set it up correctly.
> The use of a dynamic symbol is too fragile and seems to have been added
> as an afterthought.  This should be a dynamic tag in the main
> executable.  If we had that, we could set GP in all relevant calls to
> application code (of course this assumes that applications do not use GP
> as a global register variable for a different purpose, but hopefully
> that's not the case).
> 
> Thanks,
> Florian

Thanks for clarifying that!

I proposed an idea to bypass the GP initialization by restricting
the use of the norelax attribute for all callees of IFUNC. In this
case, the linker will retain auipc + load/store instructions to
access global variables based on pc-relative address mode instead
of relaxing them to a single load/store at a +- 2KB offset from the
GP.

I'm not sure if this solution is the right way to go. Are there any
potential edge cases that I might have missed?

Thanks,
Yangyu Chen


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] RISC-V: Fix IFUNC resolver cannot access gp pointer
  2024-10-30 18:28                   ` Yangyu Chen
@ 2024-10-31 10:07                     ` Kito Cheng
  2024-10-31 10:32                     ` Florian Weimer
  1 sibling, 0 replies; 16+ messages in thread
From: Kito Cheng @ 2024-10-31 10:07 UTC (permalink / raw)
  To: Yangyu Chen
  Cc: Florian Weimer, Kito Cheng, Palmer Dabbelt, libc-alpha,
	vincent.chen, schwab

On Thu, Oct 31, 2024 at 2:29 AM Yangyu Chen <cyy@cyyself.name> wrote:
>
>
>
> > On Oct 30, 2024, at 19:09, Florian Weimer <fweimer@redhat.com> wrote:
> >
> > * Kito Cheng:
> >
> >> Hi Florian:
> >>
> >> Thanks for your explanation! one more question around the
> >> HIDDEN_VAR_NEEDS_DYNAMIC_RELOC,
> >> RISC-V don't set that to 1 but seems to work well so far...?
> >> Is it because the GP register is limited to executable so that we are
> >> safe without that?
> >
> > Yes, exactly.  The dynamic linker doesn't use the GP register for its
> > own purposes, and it doesn't impact code generation for the dynamic
> > linker itself.
> >
> > However, the way GP setup is currently handled is highly problematic
> > because the ABI does not give us a proper way to set it up correctly.
> > The use of a dynamic symbol is too fragile and seems to have been added
> > as an afterthought.  This should be a dynamic tag in the main
> > executable.  If we had that, we could set GP in all relevant calls to
> > application code (of course this assumes that applications do not use GP
> > as a global register variable for a different purpose, but hopefully
> > that's not the case).
> >
> > Thanks,
> > Florian
>
> Thanks for clarifying that!
>
> I proposed an idea to bypass the GP initialization by restricting
> the use of the norelax attribute for all callees of IFUNC. In this
> case, the linker will retain auipc + load/store instructions to
> access global variables based on pc-relative address mode instead
> of relaxing them to a single load/store at a +- 2KB offset from the
> GP.
>
> I'm not sure if this solution is the right way to go. Are there any
> potential edge cases that I might have missed?

That seems the right way to me :)


>
> Thanks,
> Yangyu Chen
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] RISC-V: Fix IFUNC resolver cannot access gp pointer
  2024-10-30 18:28                   ` Yangyu Chen
  2024-10-31 10:07                     ` Kito Cheng
@ 2024-10-31 10:32                     ` Florian Weimer
  2024-10-31 10:35                       ` Yangyu Chen
  1 sibling, 1 reply; 16+ messages in thread
From: Florian Weimer @ 2024-10-31 10:32 UTC (permalink / raw)
  To: Yangyu Chen; +Cc: Kito Cheng, Palmer Dabbelt, libc-alpha, vincent.chen, schwab

* Yangyu Chen:

> I proposed an idea to bypass the GP initialization by restricting
> the use of the norelax attribute for all callees of IFUNC. In this
> case, the linker will retain auipc + load/store instructions to
> access global variables based on pc-relative address mode instead
> of relaxing them to a single load/store at a +- 2KB offset from the
> GP.

What do you mean by “all callees of IFUNC”?  This issue only affects the
resolver, not the IFUNC implementation itself.

As I said, if the glibc dynamic linker has a reliable way to identify
the expected GP value, it can set GP before calling the resolver.  That
requires some form of ABI change, but it would be backwards compatible.
(We just won't be able to do it if the binary doesn't contain the required
information, and the __global_pointer$ dynamic symbol is absent as
well.)

The technical side should be easy to implement.

Thanks,
Florian


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] RISC-V: Fix IFUNC resolver cannot access gp pointer
  2024-10-31 10:32                     ` Florian Weimer
@ 2024-10-31 10:35                       ` Yangyu Chen
  2024-10-31 10:51                         ` Florian Weimer
  0 siblings, 1 reply; 16+ messages in thread
From: Yangyu Chen @ 2024-10-31 10:35 UTC (permalink / raw)
  To: Florian Weimer
  Cc: Kito Cheng, Palmer Dabbelt, libc-alpha, vincent.chen, schwab



> On Oct 31, 2024, at 18:32, Florian Weimer <fweimer@redhat.com> wrote:
> 
> * Yangyu Chen:
> 
>> I proposed an idea to bypass the GP initialization by restricting
>> the use of the norelax attribute for all callees of IFUNC. In this
>> case, the linker will retain auipc + load/store instructions to
>> access global variables based on pc-relative address mode instead
>> of relaxing them to a single load/store at a +- 2KB offset from the
>> GP.
> 
> What do you mean by “all callees of IFUNC”?  This issue only affects the
> resolver, not the IFUNC implementation itself.

Oh… I mean the IFUNC resolver generated by compilers for target_clones
feature, which is under the control of the compiler.

> 
> As I said, if the glibc dynamic linker has a reliable way to identify
> the expected GP value, it can set GP before calling the resolver.  That
> requires some form of ABI change, but it would be backwards compatible.
> (We just won't be able to do it if the binary doesn't contain the required
> information, and the __global_pointer$ dynamic symbol is absent as
> well.)
> 
> The technical side should be easy to implement.
> 
> Thanks,
> Florian


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] RISC-V: Fix IFUNC resolver cannot access gp pointer
  2024-10-31 10:35                       ` Yangyu Chen
@ 2024-10-31 10:51                         ` Florian Weimer
  0 siblings, 0 replies; 16+ messages in thread
From: Florian Weimer @ 2024-10-31 10:51 UTC (permalink / raw)
  To: Yangyu Chen; +Cc: Kito Cheng, Palmer Dabbelt, libc-alpha, vincent.chen, schwab

* Yangyu Chen:

>> On Oct 31, 2024, at 18:32, Florian Weimer <fweimer@redhat.com> wrote:
>> 
>> * Yangyu Chen:
>> 
>>> I proposed an idea to bypass the GP initialization by restricting
>>> the use of the norelax attribute for all callees of IFUNC. In this
>>> case, the linker will retain auipc + load/store instructions to
>>> access global variables based on pc-relative address mode instead
>>> of relaxing them to a single load/store at a +- 2KB offset from the
>>> GP.
>> 
>> What do you mean by “all callees of IFUNC”?  This issue only affects the
>> resolver, not the IFUNC implementation itself.
>
> Oh… I mean the IFUNC resolver generated by compilers for target_clones
> feature, which is under the control of the compiler.

Ah, right, for compiler-generated resolvers for target clones, this is
indeed easier to accomplish.

Thanks,
Florian


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: Re: [PATCH v2] RISC-V: Fix IFUNC resolver cannot access gp pointer
  2024-10-30  3:10   ` Palmer Dabbelt
  2024-10-30  6:50     ` Yangyu Chen
@ 2024-11-13 10:29     ` Yangyu Chen
  1 sibling, 0 replies; 16+ messages in thread
From: Yangyu Chen @ 2024-11-13 10:29 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: libc-alpha, kito.cheng, vincent.chen, schwab, Jessica Clarke,
	Vivian Wang, Andrew Waterman, Piyou Chen



On 10/30/24 11:10, Palmer Dabbelt wrote:
>> On 10/16/24 01:01, Yangyu Chen wrote:
>>> In some cases, an IFUNC resolver may need to access the gp pointer to
>>> resolve the function address. Such an object may have l_relocated == 0.
>>> In this case, the GP register will not be set up. Thus, the IFUNC
>>> resolver cannot access the gp pointer. This patch fixes this issue by
>>> relaxing the check of l_relocated in elf_machine_runtime_setup.
>>>
>>> As for the original Bug 31317, since the static-linked executable has
>>> already set up the gp pointer, we don't need to execute the code to set
>>> up the gp pointer again. I have also reproduced and checked Bug 31317,
>>> this patch can fix the issue.
> 
> We had a pretty similar issue with the hwprobe IFUNC stuff, and IIRC the 
> result there was that it's just not safe to look up global symbols from 
> an IFUNC.
> 

Hi Palmer,

I think there is no ABI grey area for dynamically linked executables 
since we have the following statements in the riscv-elf doc [1]:

`__global_pointer$` must be exported in the dynamic symbol table of 
dynamically linked executables if there are any GP-relative accesses in 
the executable.

And after some discussions [2] about adding norelax attribute in the 
riscv-c-api-doc, I think this patch is the right way to go.

How do you think about this explanation?

Thanks,
Yangyu Chen

[1] 
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blame/3799de41be668bc42cad7fac9ccc28d20f3fcb5e/riscv-elf.adoc#L389
[2] https://github.com/riscv-non-isa/riscv-c-api-doc/pull/94

>>> Closes: BZ #32269
>>> Fixes: 96d1b9ac23 ("RISC-V: Fix the static-PIE non-relocated object 
>>> check")
>>> Signed-off-by: Yangyu Chen <cyy@cyyself.name>
>>> ---
>>>   sysdeps/riscv/dl-machine.h | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/sysdeps/riscv/dl-machine.h b/sysdeps/riscv/dl-machine.h
>>> index b2f28697f7..10a36d6701 100644
>>> --- a/sysdeps/riscv/dl-machine.h
>>> +++ b/sysdeps/riscv/dl-machine.h
>>> @@ -348,7 +348,8 @@ elf_machine_runtime_setup (struct link_map *l, 
>>> struct r_scope_elem *scope[],
>>>         gotplt[1] = (ElfW(Addr)) l;
>>>       }
>>>
>>> -  if (l->l_type == lt_executable && l->l_relocated)
>>> +#ifdef SHARED
>>> +  if (l->l_type == lt_executable)
>>>       {
>>>         /* The __global_pointer$ may not be defined by the linker if the
>>>        $gp register does not be used to access the global variable
>>> @@ -368,6 +369,7 @@ elf_machine_runtime_setup (struct link_map *l, 
>>> struct r_scope_elem *scope[],
>>>             : "r" (ref->st_value)
>>>           );
>>>       }
>>> +#endif
>>>   #endif
>>>     return lazy;
>>>   }
> 



^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2024-11-13 10:29 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-10-15 17:01 [PATCH v2] RISC-V: Fix IFUNC resolver cannot access gp pointer Yangyu Chen
2024-10-30  2:58 ` Yangyu Chen
2024-10-30  3:10   ` Palmer Dabbelt
2024-10-30  6:50     ` Yangyu Chen
2024-10-30  7:36       ` Kito Cheng
2024-10-30  9:22         ` Florian Weimer
2024-10-30 10:26           ` Kito Cheng
2024-10-30 10:42             ` Florian Weimer
2024-10-30 10:48               ` Kito Cheng
2024-10-30 11:09                 ` Florian Weimer
2024-10-30 18:28                   ` Yangyu Chen
2024-10-31 10:07                     ` Kito Cheng
2024-10-31 10:32                     ` Florian Weimer
2024-10-31 10:35                       ` Yangyu Chen
2024-10-31 10:51                         ` Florian Weimer
2024-11-13 10:29     ` Yangyu Chen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).