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

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

[Manjunath Matti]

Add support in PowerPC to use sysconf (_SC_SIGSTKSZ) to set SIGSTKSZ
and MINSIGSTKSZ similar to x86. The minimum signal stack size derived
from AT_MINSIGSTKSZ, which is the minimum number of bytes of free stack
space required in order to gurantee successful, non-nested handling
of a single signal whose handler is an empty function.

The kernel commit 2896b2dff49d0377e4372f470dcddbcb26f2be59 implements
AT_MINSIGSTKSZ AUXV entry, allowing userspace to dynamically size
stack allocations.
---
 .../unix/sysv/linux/powerpc/bits/sigstksz.h   | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h

diff --git a/sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h b/sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h
new file mode 100644
index 0000000000..399a49c1b0
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h
@@ -0,0 +1,33 @@
+/* Definition of MINSIGSTKSZ and SIGSTKSZ.  Linux/PowerPC version.
+   Copyright (C) 2023 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#ifndef _SIGNAL_H
+# error "Never include <bits/sigstksz.h> directly; use <signal.h> instead."
+#endif
+
+#if defined __USE_DYNAMIC_STACK_SIZE && __USE_DYNAMIC_STACK_SIZE
+# include <unistd.h>
+
+/* Default stack size for a signal handler: sysconf (SC_SIGSTKSZ).  */
+# undef SIGSTKSZ
+# define SIGSTKSZ sysconf (_SC_SIGSTKSZ)
+
+/* Minimum stack size for a signal handler: SIGSTKSZ/4.  */
+# undef MINSIGSTKSZ
+# define MINSIGSTKSZ (SIGSTKSZ >> 2)
+#endif
-- 
2.37.2

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

[Florian Weimer]

* Manjunath Matti via Libc-alpha:

> diff --git a/sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h b/sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h
> new file mode 100644
> index 0000000000..399a49c1b0
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h
> @@ -0,0 +1,33 @@

> +#ifndef _SIGNAL_H
> +# error "Never include <bits/sigstksz.h> directly; use <signal.h> instead."
> +#endif
> +
> +#if defined __USE_DYNAMIC_STACK_SIZE && __USE_DYNAMIC_STACK_SIZE
> +# include <unistd.h>
> +
> +/* Default stack size for a signal handler: sysconf (SC_SIGSTKSZ).  */
> +# undef SIGSTKSZ
> +# define SIGSTKSZ sysconf (_SC_SIGSTKSZ)
> +
> +/* Minimum stack size for a signal handler: SIGSTKSZ/4.  */
> +# undef MINSIGSTKSZ
> +# define MINSIGSTKSZ (SIGSTKSZ >> 2)
> +#endif

I'm not sure if it is a good idea to hard-code that constant 4 here.
Similarly, it's not great to encode the constant 1 in the default case.

This is a subtle portability hazard because with these changes,
MINSIGSTKSZ is not enough on POWER to do anything on the signal stack
(because it's exactly the kernel supplied value).  On other
architectures, it's at least possible to do some minor stuff and call
e.g. siglongjmp (at least if lazy binding is not required).

The factor 4 in the implementation is somewhat x86-64-specific because
it accounts for the fact that we do a signal-like context switch
(pushing XSAVE data on the stack) in the lazy binding trampoline.

Thanks,
Florian

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

[Manjunath S Matti]

On 25/05/23 12:38 am, Florian Weimer wrote:
> * Manjunath Matti via Libc-alpha:
>
>> diff --git a/sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h b/sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h
>> new file mode 100644
>> index 0000000000..399a49c1b0
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h
>> @@ -0,0 +1,33 @@
>> +#ifndef _SIGNAL_H
>> +# error "Never include <bits/sigstksz.h> directly; use <signal.h> instead."
>> +#endif
>> +
>> +#if defined __USE_DYNAMIC_STACK_SIZE && __USE_DYNAMIC_STACK_SIZE
>> +# include <unistd.h>
>> +
>> +/* Default stack size for a signal handler: sysconf (SC_SIGSTKSZ).  */
>> +# undef SIGSTKSZ
>> +# define SIGSTKSZ sysconf (_SC_SIGSTKSZ)
>> +
>> +/* Minimum stack size for a signal handler: SIGSTKSZ/4.  */
>> +# undef MINSIGSTKSZ
>> +# define MINSIGSTKSZ (SIGSTKSZ >> 2)
>> +#endif
> I'm not sure if it is a good idea to hard-code that constant 4 here.
> Similarly, it's not great to encode the constant 1 in the default case.
>
> This is a subtle portability hazard because with these changes,
> MINSIGSTKSZ is not enough on POWER to do anything on the signal stack
> (because it's exactly the kernel supplied value).  On other
> architectures, it's at least possible to do some minor stuff and call
> e.g. siglongjmp (at least if lazy binding is not required).

After deliberate thinking, internal discussions and the below comment by 
H J Lu.

https://patchwork.sourceware.org/project/glibc/patch/20230424105208.301614-1-mmatti@linux.ibm.com/#144525

I think we can drop this patch.

> The factor 4 in the implementation is somewhat x86-64-specific because
> it accounts for the fact that we do a signal-like context switch
> (pushing XSAVE data on the stack) in the lazy binding trampoline.
>
> Thanks,
> Florian
>
Thank you Florian for your time and support.

Regards,

Manjunath S Matti.

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

[H.J. Lu]

On Wed, May 17, 2023 at 3:18 PM Rajalakshmi Srinivasaraghavan
<rajis@linux.vnet.ibm.com> wrote:
>
>
> On 5/11/23 11:50 AM, Manjunath S Matti wrote:
>
>
> On 09/05/23 11:03 pm, Tulio Magno Quites Machado Filho wrote:
>
> Manjunath S Matti <mmatti@linux.vnet.ibm.com> writes:
>
> Am I missing some thing, please help me understand the file
>
> No. You're right.
> The issue I hypothesized can't happen.
>
> Thanks for confirming.
>
> The patch looks good.
>
> I will definitely add a testcases just to check what value are we getting
>
> from the kernel.
>
> I'm sorry, I'm not sure this test is needed anymore.
> While it doesn't hurt to have it, it would add little value.
>
> Thanks again!
>
> I have one last thing that I fail to understand, in x86
>
> file: sysdeps/unix/sysv/linux/bits/sigstksz.h
>
>  26 /* Default stack size for a signal handler: sysconf (SC_SIGSTKSZ).  */
>  27 # undef SIGSTKSZ
>  28 # define SIGSTKSZ sysconf (_SC_SIGSTKSZ)
>  29
>  30 /* Minimum stack size for a signal handler: SIGSTKSZ.  */
>  31 # undef MINSIGSTKSZ
>  32 # define MINSIGSTKSZ SIGSTKSZ
>
> line number 32, both MINSIGSTKSZ and SIGSTKSZ are intentionally set to the same value.
>
> Do you know why, I just wanted to know the reason behind this.
>
>
> This was added by commit 6c57d320484988e87e446e2e60ce42816bf51d53.
>
> @H.J. Lu Do you have any comments on this question?
>

There are

@item _SC_MINSIGSTKSZ
@standards{GNU, unistd.h}
Inquire about the minimum number of bytes of free stack space required
in order to guarantee successful, non-nested handling of a single signal
whose handler is an empty function.

@item _SC_SIGSTKSZ
@standards{GNU, unistd.h}
Inquire about the suggested minimum number of bytes of stack space
required for a signal stack.

This is not guaranteed to be enough for any specific purpose other than
the invocation of a single, non-nested, empty handler, but nonetheless
should be enough for basic scenarios involving simple signal handlers
and very low levels of signal nesting (say, 2 or 3 levels at the very
most).

This value is provided for developer convenience and to ease migration
from the legacy @code{SIGSTKSZ} constant.  Programs requiring stronger
guarantees should avoid using it if at all possible.

@vtable @code
@item SIGSTKSZ
This is the canonical size for a signal stack.  It is judged to be
sufficient for normal uses.

@item MINSIGSTKSZ
This is the amount of signal stack space the operating system needs just
to implement signal delivery.  The size of a signal stack @strong{must}
be greater than this.

For most cases, just using @code{SIGSTKSZ} for @code{ss_size} is
sufficient.  But if you know how much stack space your program's signal
handlers will need, you may want to use a different size.  In this case,
you should allocate @code{MINSIGSTKSZ} additional bytes for the signal
stack and increase @code{ss_size} accordingly.

Since sysconf (_SC_SIGSTKSZ) is the suggested minimum signal stack size,
it is used for both SIGSTKSZ and MINSIGSTKSZ.


-- 
H.J.

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

[Rajalakshmi Srinivasaraghavan]

[-- Attachment #1: Type: text/plain, Size: 1301 bytes --]


On 5/11/23 11:50 AM, Manjunath S Matti wrote:
>
> On 09/05/23 11:03 pm, Tulio Magno Quites Machado Filho wrote:
>> Manjunath S Matti <mmatti@linux.vnet.ibm.com> writes:
>>
>>> Am I missing some thing, please help me understand the file
>> No. You're right.
>> The issue I hypothesized can't happen.
> Thanks for confirming.

The patch looks good.

>>> I will definitely add a testcases just to check what value are we 
>>> getting
>>>
>>> from the kernel.
>> I'm sorry, I'm not sure this test is needed anymore.
>> While it doesn't hurt to have it, it would add little value.
>>
>> Thanks again!
>>
> I have one last thing that I fail to understand, in x86
>
> file: sysdeps/unix/sysv/linux/bits/sigstksz.h
>
>  26 /* Default stack size for a signal handler: sysconf 
> (SC_SIGSTKSZ).  */
>  27 # undef SIGSTKSZ
>  28 # define SIGSTKSZ sysconf (_SC_SIGSTKSZ)
>  29
>  30 /* Minimum stack size for a signal handler: SIGSTKSZ.  */
>  31 # undef MINSIGSTKSZ
>  32 # define MINSIGSTKSZ SIGSTKSZ
>
> line number 32, both MINSIGSTKSZ and SIGSTKSZ are intentionally set to 
> the same value.
>
> Do you know why, I just wanted to know the reason behind this.

This was added by commit 6c57d320484988e87e446e2e60ce42816bf51d53.

@H.J. Lu Do you have any comments on this question?

>
> Thank you
>
>

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

[Manjunath S Matti]

On 09/05/23 11:03 pm, Tulio Magno Quites Machado Filho wrote:
> Manjunath S Matti <mmatti@linux.vnet.ibm.com> writes:
>
>> Am I missing some thing, please help me understand the file
> No. You're right.
> The issue I hypothesized can't happen.
Thanks for confirming.
>> I will definitely add a testcases just to check what value are we getting
>>
>> from the kernel.
> I'm sorry, I'm not sure this test is needed anymore.
> While it doesn't hurt to have it, it would add little value.
>
> Thanks again!
>
I have one last thing that I fail to understand, in x86

file: sysdeps/unix/sysv/linux/bits/sigstksz.h

  26 /* Default stack size for a signal handler: sysconf (SC_SIGSTKSZ).  */
  27 # undef SIGSTKSZ
  28 # define SIGSTKSZ sysconf (_SC_SIGSTKSZ)
  29
  30 /* Minimum stack size for a signal handler: SIGSTKSZ.  */
  31 # undef MINSIGSTKSZ
  32 # define MINSIGSTKSZ SIGSTKSZ

line number 32, both MINSIGSTKSZ and SIGSTKSZ are intentionally set to 
the same value.

Do you know why, I just wanted to know the reason behind this.

Thank you

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

[Tulio Magno Quites Machado Filho]

Manjunath S Matti <mmatti@linux.vnet.ibm.com> writes:

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

No. You're right.
The issue I hypothesized can't happen.

> I will definitely add a testcases just to check what value are we getting
>
> from the kernel.

I'm sorry, I'm not sure this test is needed anymore.
While it doesn't hurt to have it, it would add little value.

Thanks again!

-- 
Tulio Magno

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

[Manjunath S Matti]

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!

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

[Tulio Magno Quites Machado Filho]

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.

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.

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

Thanks for elaborating your explanation!
That was really helpful.

-- 
Tulio Magno

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

[Manjunath S Matti]

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.

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

[Tulio Magno Quites Machado Filho]

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?

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.

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

-- 
Tulio Magno

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

[Manjunath S Matti]

[-- Attachment #1: Type: text/plain, Size: 3102 bytes --]


On 28/04/23 11:35 pm, Tulio Magno Quites Machado Filho wrote:
> Manjunath Matti via Libc-alpha<libc-alpha@sourceware.org>  writes:
>
>> Add support in PowerPC to use sysconf (_SC_SIGSTKSZ) to set SIGSTKSZ
>> and MINSIGSTKSZ similar to x86.
> This commit message explains what is being done, but it doesn't make it
> clear why this commit is important.
> If I understand correctly, the goal is to have dynamic values for the
> signal stack size and minimum signal stack size. Is this correct?

Yes that is correct,

file: sysdeps/unix/sysv/linux/dl-parse_auxv.h

  52   _dl_random = (void *) auxv_values[AT_RANDOM];
  53   GLRO(dl_minsigstacksize) = auxv_values[AT_MINSIGSTKSZ]; <-- gets 
the value from kernel.
  54   GLRO(dl_sysinfo_dso) = (void *) auxv_values[AT_SYSINFO_EHDR];

> I also suggest to mention the kernel commit ID that started doing this:
> 2896b2dff49d0377e4372f470dcddbcb26f2be59
Yes I will add it.
>> diff --git a/sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h b/sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h
>> new file mode 100644
>> index 0000000000..2bec1e7917
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h
>> @@ -0,0 +1,33 @@
>> +/* Definition of MINSIGSTKSZ and SIGSTKSZ.  Linux/PowerPC version.
>> +   Copyright (C) 2020-2023 Free Software Foundation, Inc.
>                      ^ I believe this should be just 2023.
Ack.
>
>> +#if defined __USE_DYNAMIC_STACK_SIZE && __USE_DYNAMIC_STACK_SIZE
>> +# include <unistd.h>
>> +
>> +/* Default stack size for a signal handler: sysconf (SC_SIGSTKSZ).  */
>> +# undef SIGSTKSZ
>> +# define SIGSTKSZ sysconf (_SC_SIGSTKSZ)
> I have been told that structs could be using SIGSTKSZ to set their size.
> If that's happening, the softwares using them will stop building after this
> change.
> Is this reasonable?
>
>> +/* 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.

> 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


I will resubmit my patch with the changes I have acknowledged.


Thanks for your time and support.

Manjunath Matti.

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

[Tulio Magno Quites Machado Filho]

Manjunath Matti via Libc-alpha <libc-alpha@sourceware.org> writes:

> Add support in PowerPC to use sysconf (_SC_SIGSTKSZ) to set SIGSTKSZ
> and MINSIGSTKSZ similar to x86.

This commit message explains what is being done, but it doesn't make it
clear why this commit is important.
If I understand correctly, the goal is to have dynamic values for the
signal stack size and minimum signal stack size. Is this correct?

I also suggest to mention the kernel commit ID that started doing this:
2896b2dff49d0377e4372f470dcddbcb26f2be59

> diff --git a/sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h b/sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h
> new file mode 100644
> index 0000000000..2bec1e7917
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h
> @@ -0,0 +1,33 @@
> +/* Definition of MINSIGSTKSZ and SIGSTKSZ.  Linux/PowerPC version.
> +   Copyright (C) 2020-2023 Free Software Foundation, Inc.

                    ^ I believe this should be just 2023.

> +#if defined __USE_DYNAMIC_STACK_SIZE && __USE_DYNAMIC_STACK_SIZE
> +# include <unistd.h>
> +
> +/* Default stack size for a signal handler: sysconf (SC_SIGSTKSZ).  */
> +# undef SIGSTKSZ
> +# define SIGSTKSZ sysconf (_SC_SIGSTKSZ)

I have been told that structs could be using SIGSTKSZ to set their size.
If that's happening, the softwares using them will stop building after this
change.
Is this reasonable?

> +/* 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.

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.

-- 
Tulio Magno

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

[Manjunath Matti]

Add support in PowerPC to use sysconf (_SC_SIGSTKSZ) to set SIGSTKSZ
and MINSIGSTKSZ similar to x86.
---
 .../unix/sysv/linux/powerpc/bits/sigstksz.h   | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h

diff --git a/sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h b/sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h
new file mode 100644
index 0000000000..2bec1e7917
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/powerpc/bits/sigstksz.h
@@ -0,0 +1,33 @@
+/* Definition of MINSIGSTKSZ and SIGSTKSZ.  Linux/PowerPC version.
+   Copyright (C) 2020-2023 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#ifndef _SIGNAL_H
+# error "Never include <bits/sigstksz.h> directly; use <signal.h> instead."
+#endif
+
+#if defined __USE_DYNAMIC_STACK_SIZE && __USE_DYNAMIC_STACK_SIZE
+# include <unistd.h>
+
+/* Default stack size for a signal handler: sysconf (SC_SIGSTKSZ).  */
+# undef SIGSTKSZ
+# define SIGSTKSZ sysconf (_SC_SIGSTKSZ)
+
+/* Minimum stack size for a signal handler: SIGSTKSZ/4.  */
+# undef MINSIGSTKSZ
+# define MINSIGSTKSZ (SIGSTKSZ >> 2)
+#endif
-- 
2.37.2