Re: [PATCH] powerpc: Use sysconf (_SC_SIGSTKSZ) to set SIGSTKSZ and MINSIGSTKSZ.

[Manjunath S Matti <mmatti@linux.vnet.ibm.com>]

On 05/05/23 7:44 pm, Tulio Magno Quites Machado Filho wrote:
> Manjunath S Matti <mmatti@linux.vnet.ibm.com> writes:
>
>> On 03/05/23 11:18 pm, Tulio Magno Quites Machado Filho wrote:
>>> Manjunath S Matti <mmatti@linux.vnet.ibm.com> writes:
>>>
>>>> On 28/04/23 11:35 pm, Tulio Magno Quites Machado Filho wrote:
>>>>> Manjunath Matti via Libc-alpha<libc-alpha@sourceware.org>  writes:
>>>>>
>>>>>> +/* Minimum stack size for a signal handler: SIGSTKSZ/4.  */
>>>>>> +# undef MINSIGSTKSZ
>>>>>> +# define MINSIGSTKSZ (SIGSTKSZ >> 2)
>>>>>> +#endif
>>>>> I didn't understand this part.
>>>>> Why SIGSTKSZ/4 ? I know this is correct now, but I think the kernel is
>>>>> allowed to use another value.
>>>>> Why is this part not using sysconf(_SC_MINSIGSTKSZ)?
>>>>> I'm not suggesting to use sysconf() here, but I'm trying to understand
>>>>> why the same source of value for both SIGSTKSZ and MINSIGSTKSZ is not
>>>>> being used.
>>>> In file: sysdeps/unix/sysv/linux/sysconf-sigstksz.h
>>>>
>>>>     28   if (minsigstacksize < MINSIGSTKSZ)
>>>>     29     minsigstacksize = MINSIGSTKSZ;
>>>>     30   /* MAX (MINSIGSTKSZ, sysconf (_SC_MINSIGSTKSZ)) * 4.  */
>>>>     31   long int sigstacksize = minsigstacksize * 4;
>>>>
>>>> So we are not changing the default implementation.
>>> I'm not sure I understood you. Are you trying to tell me that you want
>>> sysconf_sigstksz() to continue to return the same result?
>> Do you want me to implement a powerpc specific function ?
>>> If this is the case, be careful with the creation of
>>> sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h because it is an
>>> installed header. That means the values that are being set here will leak
>>> to user code if __USE_DYNAMIC_STACK_SIZE is defined.
>>>
>>> If that happens, user code may end up having
>>> MINSIGSTKSZ != getauxval(AT_MINSIGSTKSZ) if the kernel decides to change
>>> the value of AT_MINSIGSTKSZ.
>> My observation is that, MINSIGSTKSZ is not the same as
>> getauxval(AT_MINSIGSTKSZ).
> OK.  And I'm trying to warn you there is a risk of having
> MINSIGSTKSZ < getauxval(AT_MINSIGSTKSZ) when __USE_DYNAMIC_STACK_SIZE
> is defined.

Am I missing some thing, please help me understand the file

sysdeps/unix/sysv/linux/sysconf-sigstksz.h

sets up the value of minsigstacksize =_dl_minsigstacksize and

then compares if it is less than MINSIGSTKSZ line no 28.

then sig

  24   long int minsigstacksize = GLRO(dl_minsigstacksize);
  25   assert (minsigstacksize != 0);
  26   _Static_assert (__builtin_constant_p (MINSIGSTKSZ),
  27                   "MINSIGSTKSZ is constant");
  28   if (minsigstacksize < MINSIGSTKSZ)
  29     minsigstacksize = MINSIGSTKSZ;

  30   /* MAX (MINSIGSTKSZ, sysconf (_SC_MINSIGSTKSZ)) * 4.  */
  31   long int sigstacksize = minsigstacksize * 4;
  32   /* Return MAX (SIGSTKSZ, sigstacksize).  */
  33   _Static_assert (__builtin_constant_p (SIGSTKSZ),
  34                   "SIGSTKSZ is constant");
  35   if (sigstacksize < SIGSTKSZ)
  36     sigstacksize = SIGSTKSZ;
  37   return sigstacksize;

> I'm afraid we're diverging from the original discussion, which is:
> the minimum stack size for a signal handler is calculated from the
> amount of data the kernel needs to save in the stack.
>
> The kernel calculates that and provide it via getauxval(AT_MINSIGSTKSZ).
> AFAIU, calculating the minimum stack size for a signal handler based on
> getauxval(AT_SIGSTKSZ) may lead to errors because there are no
> guarantees that getauxval(AT_SIGSTKSZ)/4 > getauxval(AT_MINSIGSTKSZ),
> even if that is true now, it isn't future proof.

We are infact deriving sigstacksize value from minsigstacksize * 4,

line number 30, 32, 35 to 37.

> Besides that, the test I suggested to implement would guarantee that
> glibc code remains up-to-date according to the interpretation that is
> adopted.

I will definitely add a testcases just to check what value are we getting

from the kernel.

> Thanks for elaborating your explanation!
> That was really helpful.
>
Thank you for helping me out here!