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

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

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).

Either in case of PowerPC or in case of x86. Let me try to explain

getauxval(AT_MINSIGSTKSZ) returns 4224 which is the signal frame size in 
the kernel

commit 63dee5df43a31f3844efabc58972f0a206ca4534 , whereas

commit 2f82ec19757f58549467db568c56e7dfff8af283 Increase MINSIGSTKSZ to 
8192.

So MINSIGSTKSZ cannot take a value less than 8192,  testcase 
"signal/tst-minsigstksz-5.c"

fails. Similar behaviour has been observed in x86, both 
getauxval(AT_MINSIGSTKSZ) and

sysconf (_SC_MINSIGSTKSZ) return 1776 and MINSIGSTKSZ is 8192.

>>> If we reach consensus that both macros in this file can have values set
>>> at runtime, then I it might be worth adding a test in order to check that
>>> dl_minsigstacksize, MINSIGSTKSZ and AT_MINSIGSTKSZ passed by the kernel
>>> are identical.
>>>
>> There are testcases which already use MINSIGSTKSZ
>>
>> sysdeps/pthread/tst-signal6.c
>>
>> signal/tst-minsigstksz-5.c
> AFAICS none of these tests are checking if dl_minsigstacksize, MINSIGSTKSZ and
> AT_MINSIGSTKSZ have identical values.
> AFAIU, at least on powerpc, they should always be identical.
> Having such a test would help:
> - to signal if the kernel changes it without a warning.
> - if another commit changed one of them by mistake.
>
> AFAIU, the tests you pointed out help to identify if the current sizes
> are indeed enough to handle signals, which is also very important.
>
As per the above explaination, MINSIGSTKSZ and sysconf (_SC_MINSIGSTKSZ) 
do not have the same values.