[2/2] jmpbuf: Add paddings for target specific usage

Message ID 20171107223908.GB46190@intel.com
State New
Headers show
Series
  • [1/2] Add tst-jmp_buf.c and jmp_buf-macros.h
Related show

Commit Message

H.J. Lu Nov. 7, 2017, 10:39 p.m.
To support Shadow Stack (SHSTK) in Intel Control-flow Enforcement
Technology (CET) in setjmp/longjmp, we need to save shadow stack
pointer in jmp_buf.  The __saved_mask field in jmp_buf has type
of __sigset_t.  On Linux, __sigset_t is defined as

 #define _SIGSET_NWORDS (1024 / (8 * sizeof (unsigned long int)))
typedef struct
{
  unsigned long int __val[_SIGSET_NWORDS];
} __sigset_t;

which is much bigger than expected by the __sigprocmask system call,
which has

typedef struct {
        unsigned long sig[_NSIG_WORDS];
} sigset_t;

We can shrink __sigset_t used by __saved_mask in jmp_buf to add paddings
for target specific usage.  As long as the new __sigset_t is not smaller
than sigset_t expected by the __sigprocmask system call, it should work
correctly.  This patch defines __jmp_buf_sigset_t for __saved_mask in
jmp_buf on Linux and verifies __jmp_buf_sigset_t has the suitable size
for the __sigprocmask system call.

Tested with build-many-glibcs.py.

OK for master?

H.J.
---
	* bits/setjmp3.h: New file.
	* sysdeps/unix/sysv/linux/bits/setjmp3.h: Likewise.
	* setjmp/Makefile (headers): Add bits/setjmp3.h.
	* setjmp/longjmp.c (__libc_siglongjmp): Cast &env[0].__saved_mask
	to "sigset_t *".
	* setjmp/sigjmp.c (__sigjmp_save): Likewise.
	* sysdeps/powerpc/longjmp.c (__vmx__libc_siglongjmp): Likewise.
	* sysdeps/powerpc/novmx-longjmp.c (__novmx__libc_siglongjmp):
	Likewise.
	* sysdeps/powerpc/novmx-sigjmp.c (__novmx__sigjmp_save): Likewise.
	* sysdeps/powerpc/sigjmp.c (__vmx__sigjmp_save): Likewise.
	* sysdeps/unix/sysv/linux/ia64/unwind_longjmp.c
	(__libc_unwind_longjmp): Likewise.
	* setjmp/setjmp.h: Include <bits/setjmp3.h> instead of
	<bits/types/__sigset_t.h>.
	(__jmp_buf_tag): Change __saved_mask to __target.
	* setjmp/tst-jmp_buf.c: Include <signal.h>.
	[_NSIG] (KERNEL_NSIG_WORDS): New.
	[_NSIG] (kernel_sigset_t): Likewise.
	[_NSIG] (do_test): Add _Static_assert for size of __saved_mask
	in jmp_buf >= sizeof kernel_sigset_t.
---
 bits/setjmp3.h                                | 31 +++++++++++++++++
 setjmp/Makefile                               |  2 +-
 setjmp/longjmp.c                              |  3 +-
 setjmp/setjmp.h                               |  4 +--
 setjmp/sigjmp.c                               |  2 +-
 setjmp/tst-jmp_buf.c                          | 17 ++++++++++
 sysdeps/powerpc/longjmp.c                     |  3 +-
 sysdeps/powerpc/novmx-longjmp.c               |  3 +-
 sysdeps/powerpc/novmx-sigjmp.c                |  2 +-
 sysdeps/powerpc/sigjmp.c                      |  2 +-
 sysdeps/unix/sysv/linux/bits/setjmp3.h        | 48 +++++++++++++++++++++++++++
 sysdeps/unix/sysv/linux/ia64/unwind_longjmp.c |  3 +-
 12 files changed, 110 insertions(+), 10 deletions(-)
 create mode 100644 bits/setjmp3.h
 create mode 100644 sysdeps/unix/sysv/linux/bits/setjmp3.h

Comments

Florian Weimer Nov. 8, 2017, 6:05 a.m. | #1
On 11/07/2017 11:39 PM, H.J. Lu wrote:
> +typedef struct
> +  {
> +    __sigset_t __saved_mask;
> +  } __jmpbuf_target_t;

“Target” in the sense of “target architecture”, not “jump target”? 
Maybe it's better to pick a different name.

Should <bits/setjmp3.h> be a header under bits/types/ instead?

Thanks,
Florian
H.J. Lu Nov. 8, 2017, 6:27 p.m. | #2
On Tue, Nov 7, 2017 at 10:05 PM, Florian Weimer <fweimer@redhat.com> wrote:
> On 11/07/2017 11:39 PM, H.J. Lu wrote:
>>
>> +typedef struct
>> +  {
>> +    __sigset_t __saved_mask;
>> +  } __jmpbuf_target_t;
>
>
> “Target” in the sense of “target architecture”, not “jump target”? Maybe
> it's better to pick a different name.

I checked it to __jmpbuf_arch_t.

> Should <bits/setjmp3.h> be a header under bits/types/ instead?
>

I renamed it to bits/types/__jmpbuf_arch_t.h.

Here is the updated patch.  Tested with build-many-glibcs.py.

Are there any comments or objections?

Thanks.
Florian Weimer Nov. 13, 2017, 1:09 p.m. | #3
On 11/08/2017 07:27 PM, H.J. Lu wrote:
> +/* The biggest signal number + 1  */
> +#define _JUMP_BUF_SIGSET_NSIG	257
> +/* Number of longs to hold all signals.  */
> +#define _JUMP_BUF_SIGSET_NWORDS \
> +  ((_JUMP_BUF_SIGSET_NSIG - 1 + 7) / (8 * sizeof (unsigned long int)))

Where does 257 come from?  65 or 129 I would understand considering the 
kernel sources, but 257 is odd.

I think it would be clearer to hard-code the array sizes and explain why 
the values where chosen in that way.

We also need a test that setprocmask does not read from the previously 
unused part.  I can move the existing next_to_fault bits to support/ if 
that would help.

Thanks,
Florian
H.J. Lu Nov. 13, 2017, 2:05 p.m. | #4
On Mon, Nov 13, 2017 at 5:09 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 11/08/2017 07:27 PM, H.J. Lu wrote:
>>
>> +/* The biggest signal number + 1  */
>> +#define _JUMP_BUF_SIGSET_NSIG  257
>> +/* Number of longs to hold all signals.  */
>> +#define _JUMP_BUF_SIGSET_NWORDS \
>> +  ((_JUMP_BUF_SIGSET_NSIG - 1 + 7) / (8 * sizeof (unsigned long int)))
>
>
> Where does 257 come from?  65 or 129 I would understand considering the
> kernel sources, but 257 is odd.

I picked 257 to leave some room
> I think it would be clearer to hard-code the array sizes and explain why the
> values where chosen in that way.
>
> We also need a test that setprocmask does not read from the previously

Did you mean "sigprocmask"?

> unused part.  I can move the existing next_to_fault bits to support/ if that
> would help.

Yes, please.

Thanks.
Florian Weimer Nov. 13, 2017, 4:34 p.m. | #5
On 11/13/2017 03:05 PM, H.J. Lu wrote:
> On Mon, Nov 13, 2017 at 5:09 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 11/08/2017 07:27 PM, H.J. Lu wrote:
>>>
>>> +/* The biggest signal number + 1  */
>>> +#define _JUMP_BUF_SIGSET_NSIG  257
>>> +/* Number of longs to hold all signals.  */
>>> +#define _JUMP_BUF_SIGSET_NWORDS \
>>> +  ((_JUMP_BUF_SIGSET_NSIG - 1 + 7) / (8 * sizeof (unsigned long int)))
>>
>>
>> Where does 257 come from?  65 or 129 I would understand considering the
>> kernel sources, but 257 is odd.

Oh.  I'm not sure if we should put this into installed header.

Maybe we can use a different approach?  Something similar to the pthread 
types?  Or just not change the external type at all and just ad some 
internal space reuse mechanism?

We had problems with people poking at supposedly invisible jmpbuf 
contents in the past, and I'm worried that adding even __ members will 
encourage that.

>> I think it would be clearer to hard-code the array sizes and explain why the
>> values where chosen in that way.
>>
>> We also need a test that setprocmask does not read from the previously
> 
> Did you mean "sigprocmask"?

Right.

>> unused part.  I can move the existing next_to_fault bits to support/ if that
>> would help.
> 
> Yes, please.

Okay, I'll move it to support/ soon.

Thanks,
Florian
H.J. Lu Nov. 13, 2017, 4:44 p.m. | #6
On Mon, Nov 13, 2017 at 8:34 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 11/13/2017 03:05 PM, H.J. Lu wrote:
>>
>> On Mon, Nov 13, 2017 at 5:09 AM, Florian Weimer <fweimer@redhat.com>
>> wrote:
>>>
>>> On 11/08/2017 07:27 PM, H.J. Lu wrote:
>>>>
>>>>
>>>> +/* The biggest signal number + 1  */
>>>> +#define _JUMP_BUF_SIGSET_NSIG  257
>>>> +/* Number of longs to hold all signals.  */
>>>> +#define _JUMP_BUF_SIGSET_NWORDS \
>>>> +  ((_JUMP_BUF_SIGSET_NSIG - 1 + 7) / (8 * sizeof (unsigned long int)))
>>>
>>>
>>>
>>> Where does 257 come from?  65 or 129 I would understand considering the
>>> kernel sources, but 257 is odd.
>
>
> Oh.  I'm not sure if we should put this into installed header.
>
> Maybe we can use a different approach?  Something similar to the pthread
> types?  Or just not change the external type at all and just ad some
> internal space reuse mechanism?

I will see what I can do.

> We had problems with people poking at supposedly invisible jmpbuf contents
> in the past, and I'm worried that adding even __ members will encourage
> that.
>
>>> I think it would be clearer to hard-code the array sizes and explain why
>>> the
>>> values where chosen in that way.
>>>
>>> We also need a test that setprocmask does not read from the previously
>>
>>
>> Did you mean "sigprocmask"?
>
>
> Right.
>
>>> unused part.  I can move the existing next_to_fault bits to support/ if
>>> that
>>> would help.
>>
>>
>> Yes, please.
>
>
> Okay, I'll move it to support/ soon.
>

Here is what the test will look like

static int
do_test (void)
{
  struct __jmp_buf_tag *sj = memory from next_to_fault
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
"SJ" will point to the end of page in such a way that sigprocmask
will fail if it reads beyond the actual __saved_mask.

  sigset_t m;

  sigemptyset (&m);
  sigprocmask (SIG_SETMASK, &m, NULL);
  if (sigsetjmp (sj, 0) == 0)
    {
      sigaddset (&m, SIGUSR1);
      sigprocmask (SIG_SETMASK, &m, NULL);
      siglongjmp (sj, 1);
      return EXIT_FAILURE;
    }
  sigprocmask (SIG_SETMASK, NULL, &m);
  return sigismember (&m, SIGUSR1) ? EXIT_SUCCESS : EXIT_FAILURE;
}

#define TEST_FUNCTION do_test ()
#include "../test-skeleton.c"

Thanks.
H.J. Lu Nov. 13, 2017, 7:40 p.m. | #7
On Mon, Nov 13, 2017 at 8:44 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Mon, Nov 13, 2017 at 8:34 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 11/13/2017 03:05 PM, H.J. Lu wrote:
>>>
>>> On Mon, Nov 13, 2017 at 5:09 AM, Florian Weimer <fweimer@redhat.com>
>>> wrote:
>>>>
>>>> On 11/08/2017 07:27 PM, H.J. Lu wrote:
>>>>>
>>>>>
>>>>> +/* The biggest signal number + 1  */
>>>>> +#define _JUMP_BUF_SIGSET_NSIG  257
>>>>> +/* Number of longs to hold all signals.  */
>>>>> +#define _JUMP_BUF_SIGSET_NWORDS \
>>>>> +  ((_JUMP_BUF_SIGSET_NSIG - 1 + 7) / (8 * sizeof (unsigned long int)))
>>>>
>>>>
>>>>
>>>> Where does 257 come from?  65 or 129 I would understand considering the
>>>> kernel sources, but 257 is odd.
>>
>>
>> Oh.  I'm not sure if we should put this into installed header.
>>
>> Maybe we can use a different approach?  Something similar to the pthread
>> types?  Or just not change the external type at all and just ad some
>> internal space reuse mechanism?
>
> I will see what I can do.
>
>> We had problems with people poking at supposedly invisible jmpbuf contents
>> in the past, and I'm worried that adding even __ members will encourage
>> that.
>>
>>>> I think it would be clearer to hard-code the array sizes and explain why
>>>> the
>>>> values where chosen in that way.
>>>>
>>>> We also need a test that setprocmask does not read from the previously
>>>
>>>
>>> Did you mean "sigprocmask"?
>>
>>
>> Right.
>>
>>>> unused part.  I can move the existing next_to_fault bits to support/ if
>>>> that
>>>> would help.
>>>
>>>
>>> Yes, please.
>>
>>
>> Okay, I'll move it to support/ soon.
>>
>

Here is the updated patch to add <setjmpP.h>.  I added a test:

  struct support_next_to_fault jmpbuf
    = support_next_to_fault_allocate (SAVED_MASK_OFFSET + (_NSIG / 8));
  struct __jmp_buf_tag *sj = (struct __jmp_buf_tag *) jmpbuf.buffer;

  errno = 0;
  if (sigsetjmp (sj, 1) == 0)
    {
      siglongjmp (sj, 1);
      return EXIT_FAILURE;
    }
  if (errno != 0)
    {
      printf ("sigsetjmp: %s\n", strerror (errno));
      return EXIT_FAILURE;
    }

to verify that __sigprocmask won't read beyond _NSIG / 8.

Does it look OK?

Thanks.
H.J. Lu Nov. 13, 2017, 11:22 p.m. | #8
On Mon, Nov 13, 2017 at 11:40 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Mon, Nov 13, 2017 at 8:44 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Mon, Nov 13, 2017 at 8:34 AM, Florian Weimer <fweimer@redhat.com> wrote:
>>> On 11/13/2017 03:05 PM, H.J. Lu wrote:
>>>>
>>>> On Mon, Nov 13, 2017 at 5:09 AM, Florian Weimer <fweimer@redhat.com>
>>>> wrote:
>>>>>
>>>>> On 11/08/2017 07:27 PM, H.J. Lu wrote:
>>>>>>
>>>>>>
>>>>>> +/* The biggest signal number + 1  */
>>>>>> +#define _JUMP_BUF_SIGSET_NSIG  257
>>>>>> +/* Number of longs to hold all signals.  */
>>>>>> +#define _JUMP_BUF_SIGSET_NWORDS \
>>>>>> +  ((_JUMP_BUF_SIGSET_NSIG - 1 + 7) / (8 * sizeof (unsigned long int)))
>>>>>
>>>>>
>>>>>
>>>>> Where does 257 come from?  65 or 129 I would understand considering the
>>>>> kernel sources, but 257 is odd.
>>>
>>>
>>> Oh.  I'm not sure if we should put this into installed header.
>>>
>>> Maybe we can use a different approach?  Something similar to the pthread
>>> types?  Or just not change the external type at all and just ad some
>>> internal space reuse mechanism?
>>
>> I will see what I can do.
>>
>>> We had problems with people poking at supposedly invisible jmpbuf contents
>>> in the past, and I'm worried that adding even __ members will encourage
>>> that.
>>>
>>>>> I think it would be clearer to hard-code the array sizes and explain why
>>>>> the
>>>>> values where chosen in that way.
>>>>>
>>>>> We also need a test that setprocmask does not read from the previously
>>>>
>>>>
>>>> Did you mean "sigprocmask"?
>>>
>>>
>>> Right.
>>>
>>>>> unused part.  I can move the existing next_to_fault bits to support/ if
>>>>> that
>>>>> would help.
>>>>
>>>>
>>>> Yes, please.
>>>
>>>
>>> Okay, I'll move it to support/ soon.
>>>
>>
>
> Here is the updated patch to add <setjmpP.h>.  I added a test:
>
>   struct support_next_to_fault jmpbuf
>     = support_next_to_fault_allocate (SAVED_MASK_OFFSET + (_NSIG / 8));
>   struct __jmp_buf_tag *sj = (struct __jmp_buf_tag *) jmpbuf.buffer;
>
>   errno = 0;
>   if (sigsetjmp (sj, 1) == 0)
>     {
>       siglongjmp (sj, 1);
>       return EXIT_FAILURE;
>     }
>   if (errno != 0)
>     {
>       printf ("sigsetjmp: %s\n", strerror (errno));
>       return EXIT_FAILURE;
>     }
>
> to verify that __sigprocmask won't read beyond _NSIG / 8.
>
> Does it look OK?
>

This is the updated patch.  Tested with build-many-glibcs.py.

Any other comments?

Thanks.
Florian Weimer Nov. 14, 2017, 12:26 p.m. | #9
On 11/14/2017 12:22 AM, H.J. Lu wrote:
> diff --git a/setjmp/tst-sigsetjmp2.c b/setjmp/tst-sigsetjmp2.c
> new file mode 100644
> index 0000000000..131531feb0
> --- /dev/null
> +++ b/setjmp/tst-sigsetjmp2.c
> @@ -0,0 +1,50 @@
> +/* Test that sigprocmask does not read from the unused part of jmpbuf.
> +   Copyright (C) 2017 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
> +<http://www.gnu.org/licenses/>.  */
> +
> +#include <stdlib.h>
> +#include <signal.h>
> +#include <setjmp.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <jmp_buf-macros.h>
> +#include <support/next_to_fault.h>
> +
> +static int
> +do_test (void)
> +{
> +  struct support_next_to_fault jmpbuf
> +    = support_next_to_fault_allocate (SAVED_MASK_OFFSET + (_NSIG / 8));
> +  struct __jmp_buf_tag *sj = (struct __jmp_buf_tag *) jmpbuf.buffer;
> +
> +  errno = 0;
> +  if (sigsetjmp (sj, 1) == 0)
> +    {
> +      siglongjmp (sj, 1);
> +      return EXIT_FAILURE;
> +    }
> +  if (errno != 0)
> +    {
> +      printf ("sigsetjmp: %s\n", strerror (errno));
> +      return EXIT_FAILURE;
> +    }
> +  return EXIT_SUCCESS;
> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"

Please use <support/test-driver.c>.

Wouldn't the test start to fail if sigsetjmp/siglongjmp actually started 
using the padding you are freeing up for use?

I think you need test sigprocmask directly to ensure it has the desired 
property of not clobbering the padding.

Thanks,
Florian
H.J. Lu Nov. 14, 2017, 1:10 p.m. | #10
On Tue, Nov 14, 2017 at 4:26 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 11/14/2017 12:22 AM, H.J. Lu wrote:
>>
>> diff --git a/setjmp/tst-sigsetjmp2.c b/setjmp/tst-sigsetjmp2.c
>> new file mode 100644
>> index 0000000000..131531feb0
>> --- /dev/null
>> +++ b/setjmp/tst-sigsetjmp2.c
>> @@ -0,0 +1,50 @@
>> +/* Test that sigprocmask does not read from the unused part of jmpbuf.
>> +   Copyright (C) 2017 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
>> +<http://www.gnu.org/licenses/>.  */
>> +
>> +#include <stdlib.h>
>> +#include <signal.h>
>> +#include <setjmp.h>
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include <errno.h>
>> +#include <jmp_buf-macros.h>
>> +#include <support/next_to_fault.h>
>> +
>> +static int
>> +do_test (void)
>> +{
>> +  struct support_next_to_fault jmpbuf
>> +    = support_next_to_fault_allocate (SAVED_MASK_OFFSET + (_NSIG / 8));
>> +  struct __jmp_buf_tag *sj = (struct __jmp_buf_tag *) jmpbuf.buffer;
>> +
>> +  errno = 0;
>> +  if (sigsetjmp (sj, 1) == 0)
>> +    {
>> +      siglongjmp (sj, 1);
>> +      return EXIT_FAILURE;
>> +    }
>> +  if (errno != 0)
>> +    {
>> +      printf ("sigsetjmp: %s\n", strerror (errno));
>> +      return EXIT_FAILURE;
>> +    }
>> +  return EXIT_SUCCESS;
>> +}
>> +
>> +#define TEST_FUNCTION do_test ()
>> +#include "../test-skeleton.c"
>
>
> Please use <support/test-driver.c>.

Done.

> Wouldn't the test start to fail if sigsetjmp/siglongjmp actually started
> using the padding you are freeing up for use?

You are right.

> I think you need test sigprocmask directly to ensure it has the desired
> property of not clobbering the padding.

Here is the updated patch.   OK for master?

Thanks.
Florian Weimer Nov. 15, 2017, 11:10 a.m. | #11
On 11/14/2017 02:10 PM, H.J. Lu wrote:
> +static int
> +do_test (void)
> +{
> +  sigjmp_buf sj;
> +  struct support_next_to_fault sigset_t_buf
> +    = support_next_to_fault_allocate (SIZEOF_SIGSET_T);
> +  sigset_t *m_p = (sigset_t *) sigset_t_buf.buffer;
> +  sigset_t m;
> +
> +  sigemptyset (&m);
> +  memcpy (m_p, &m, SIZEOF_SIGSET_T);
> +  sigprocmask (SIG_SETMASK, m_p, NULL);
> +  memcpy (&m, m_p, SIZEOF_SIGSET_T);
> +  if (sigsetjmp (sj, 0) == 0)
> +    {
> +      sigaddset (&m, SIGUSR1);
> +      memcpy (m_p, &m, SIZEOF_SIGSET_T);
> +      sigprocmask (SIG_SETMASK, m_p, NULL);
> +      memcpy (&m, m_p, SIZEOF_SIGSET_T);
> +      siglongjmp (sj, 1);
> +      return EXIT_FAILURE;
> +    }
> +  sigprocmask (SIG_SETMASK, NULL, m_p);
> +  memcpy (&m, m_p, SIZEOF_SIGSET_T);
> +  return sigismember (&m, SIGUSR1) ? EXIT_SUCCESS : EXIT_FAILURE;
> +}

Sorry, I don't understand anymore what this test is supposed to test and 
how.

To be honest, I don't like how you inject the internal definition of 
jmp_buf.  Is this the way we do it for the nptl types?

I think you should check _JUMP_BUF_SIGSET_NSIG against a kernel constant 
(_NSIGS?) somewhere.

Thanks,
Florian
H.J. Lu Nov. 15, 2017, 1:20 p.m. | #12
On Wed, Nov 15, 2017 at 3:10 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 11/14/2017 02:10 PM, H.J. Lu wrote:
>>
>> +static int
>> +do_test (void)
>> +{
>> +  sigjmp_buf sj;
>> +  struct support_next_to_fault sigset_t_buf
>> +    = support_next_to_fault_allocate (SIZEOF_SIGSET_T);
>> +  sigset_t *m_p = (sigset_t *) sigset_t_buf.buffer;
>> +  sigset_t m;
>> +
>> +  sigemptyset (&m);
>> +  memcpy (m_p, &m, SIZEOF_SIGSET_T);
>> +  sigprocmask (SIG_SETMASK, m_p, NULL);
>> +  memcpy (&m, m_p, SIZEOF_SIGSET_T);
>> +  if (sigsetjmp (sj, 0) == 0)
>> +    {
>> +      sigaddset (&m, SIGUSR1);
>> +      memcpy (m_p, &m, SIZEOF_SIGSET_T);
>> +      sigprocmask (SIG_SETMASK, m_p, NULL);
>> +      memcpy (&m, m_p, SIZEOF_SIGSET_T);
>> +      siglongjmp (sj, 1);
>> +      return EXIT_FAILURE;
>> +    }
>> +  sigprocmask (SIG_SETMASK, NULL, m_p);
>> +  memcpy (&m, m_p, SIZEOF_SIGSET_T);
>> +  return sigismember (&m, SIGUSR1) ? EXIT_SUCCESS : EXIT_FAILURE;
>> +}
>
>
> Sorry, I don't understand anymore what this test is supposed to test and
> how.

This tests the reduced __jmp_buf_sigset_t used by __saved_mask is
bigger than sigset expected by kernel.

> To be honest, I don't like how you inject the internal definition of
> jmp_buf.  Is this the way we do it for the nptl types?

I only need to make some room in

/* Calling environment, plus possibly a saved signal mask.  */
struct __jmp_buf_tag
  {
    /* NOTE: The machine-dependent definitions of `__sigsetjmp'
       assume that a `jmp_buf' begins with a `__jmp_buf' and that
       `__mask_was_saved' follows it.  Do not move these members
       or add others before it.  */
    __jmp_buf __jmpbuf; /* Calling environment.  */
    int __mask_was_saved; /* Saved the signal mask?  */
    __sigset_t __saved_mask; /* Saved signal mask.  */
  };

for target specific purpose.   I changed it to

struct __jmp_buf_tag
  {
    /* NOTE: The machine-dependent definitions of `__sigsetjmp'
       assume that a `jmp_buf' begins with a `__jmp_buf' and that
       `__mask_was_saved' follows it.  Do not move these members
       or add others before it.  */
    __jmp_buf __jmpbuf; /* Calling environment.  */
    int __mask_was_saved; /* Saved the signal mask?  */
    union
    {
    __sigset_t __saved_mask_compat;
    struct
      {
__jmp_buf_sigset_t __saved_mask;
/* Paddings for architecture specific usage.  */
unsigned long int __padding[12];
      } __saved;
    }  __saved_mask;
  };

#define __saved_mask __saved_mask.__saved.__saved_mask

I did only to  __sigset_t  in __jmp_buf_tag and this test verifies that the size
of __jmp_buf_sigset_t works with sigprocmask.

Are you suggesting we make some room in __sigset_t directly?  This will
require very extensive changes. If not, what do you exactly suggest?

> I think you should check _JUMP_BUF_SIGSET_NSIG against a kernel constant
> (_NSIGS?) somewhere.

There is a _Static_assert in include/setjmp.h.  But it isn't triggered.  This
updated patch moves it to sysdeps/unix/sysv/linux/__saved_mask.h.   Now
I got

../sysdeps/unix/sysv/linux/__saved_mask.h:32:1: error: static
assertion failed: "size of ___saved_mask < size of
__sigprocmask_sigset_t"
 _Static_assert (sizeof (___saved_mask) >= sizeof (__sigprocmask_sigset_t),

if __jmp_buf_sigset_t is too small.

Patch

diff --git a/bits/setjmp3.h b/bits/setjmp3.h
new file mode 100644
index 0000000000..b164c8d449
--- /dev/null
+++ b/bits/setjmp3.h
@@ -0,0 +1,31 @@ 
+/* Generic __jmpbuf_target_t defition.
+   Copyright (C) 2017 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
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef _SETJMP_H
+# error "Never include <bits/setjmp3.h> directly; use <setjmp.h> instead."
+#endif
+
+#include <bits/types/__sigset_t.h>
+
+typedef struct
+  {
+    __sigset_t __saved_mask;
+  } __jmpbuf_target_t;
+
+/* Saved signal mask.  */
+#define __saved_mask __target.__saved_mask
diff --git a/setjmp/Makefile b/setjmp/Makefile
index 857e0b8d0f..6ab9b4c3ca 100644
--- a/setjmp/Makefile
+++ b/setjmp/Makefile
@@ -22,7 +22,7 @@  subdir	:= setjmp
 
 include ../Makeconfig
 
-headers	:= setjmp.h bits/setjmp.h bits/setjmp2.h
+headers	:= setjmp.h bits/setjmp.h bits/setjmp2.h bits/setjmp3.h
 
 routines	:= setjmp sigjmp bsd-setjmp bsd-_setjmp \
 		   longjmp __longjmp jmp-unwind
diff --git a/setjmp/longjmp.c b/setjmp/longjmp.c
index 2453c2c124..86a024b4b8 100644
--- a/setjmp/longjmp.c
+++ b/setjmp/longjmp.c
@@ -31,7 +31,8 @@  __libc_siglongjmp (sigjmp_buf env, int val)
 
   if (env[0].__mask_was_saved)
     /* Restore the saved signal mask.  */
-    (void) __sigprocmask (SIG_SETMASK, &env[0].__saved_mask,
+    (void) __sigprocmask (SIG_SETMASK,
+			  (sigset_t *) &env[0].__saved_mask,
 			  (sigset_t *) NULL);
 
   /* Call the machine-dependent function to restore machine state.  */
diff --git a/setjmp/setjmp.h b/setjmp/setjmp.h
index 86fb2edf6c..1082fb0142 100644
--- a/setjmp/setjmp.h
+++ b/setjmp/setjmp.h
@@ -27,7 +27,7 @@ 
 __BEGIN_DECLS
 
 #include <bits/setjmp.h>		/* Get `__jmp_buf'.  */
-#include <bits/types/__sigset_t.h>
+#include <bits/setjmp3.h>
 
 /* Calling environment, plus possibly a saved signal mask.  */
 struct __jmp_buf_tag
@@ -38,7 +38,7 @@  struct __jmp_buf_tag
        or add others before it.  */
     __jmp_buf __jmpbuf;		/* Calling environment.  */
     int __mask_was_saved;	/* Saved the signal mask?  */
-    __sigset_t __saved_mask;	/* Saved signal mask.  */
+    __jmpbuf_target_t __target;	/* Target specific data.  */
   };
 
 
diff --git a/setjmp/sigjmp.c b/setjmp/sigjmp.c
index 30839ae819..0cd733b3ee 100644
--- a/setjmp/sigjmp.c
+++ b/setjmp/sigjmp.c
@@ -28,7 +28,7 @@  __sigjmp_save (sigjmp_buf env, int savemask)
 {
   env[0].__mask_was_saved = (savemask &&
 			     __sigprocmask (SIG_BLOCK, (sigset_t *) NULL,
-					    &env[0].__saved_mask) == 0);
+					    (sigset_t *) &env[0].__saved_mask) == 0);
 
   return 0;
 }
diff --git a/setjmp/tst-jmp_buf.c b/setjmp/tst-jmp_buf.c
index 1b25d85876..16ddebc224 100644
--- a/setjmp/tst-jmp_buf.c
+++ b/setjmp/tst-jmp_buf.c
@@ -18,9 +18,19 @@ 
 
 #include <stdio.h>
 #include <setjmp.h>
+#include <signal.h>
 #include <stddef.h>
 #include <jmp_buf-macros.h>
 
+#ifdef _NSIG
+# define KERNEL_NSIG_WORDS (_NSIG / (8 * sizeof (unsigned long int)))
+
+typedef struct
+  {
+    unsigned long int __val[KERNEL_NSIG_WORDS];
+  } kernel_sigset_t;
+#endif
+
 #define STR_HELPER(x) #x
 #define STR(x) STR_HELPER(x)
 
@@ -54,6 +64,13 @@  do_test (void)
   TEST_OFFSET (struct __jmp_buf_tag, __saved_mask,
 	       SAVED_MASK_OFFSET);
 
+#ifdef _NSIG
+  jmp_buf buf;
+  __typeof (buf[0].__saved_mask) saved_mask;
+  _Static_assert (sizeof (saved_mask) >= sizeof (kernel_sigset_t),
+		  "size of saved_mask < size of kernel_sigset_t");
+#endif
+
   return 0;
 }
 
diff --git a/sysdeps/powerpc/longjmp.c b/sysdeps/powerpc/longjmp.c
index bd3ed8c22b..a23f2c582b 100644
--- a/sysdeps/powerpc/longjmp.c
+++ b/sysdeps/powerpc/longjmp.c
@@ -39,7 +39,8 @@  __vmx__libc_siglongjmp (sigjmp_buf env, int val)
 
   if (env[0].__mask_was_saved)
     /* Restore the saved signal mask.  */
-    (void) __sigprocmask (SIG_SETMASK, &env[0].__saved_mask,
+    (void) __sigprocmask (SIG_SETMASK,
+			  (sigset_t *) &env[0].__saved_mask,
 			  (sigset_t *) NULL);
 
   /* Call the machine-dependent function to restore machine state.  */
diff --git a/sysdeps/powerpc/novmx-longjmp.c b/sysdeps/powerpc/novmx-longjmp.c
index b0020b728a..662fbc5432 100644
--- a/sysdeps/powerpc/novmx-longjmp.c
+++ b/sysdeps/powerpc/novmx-longjmp.c
@@ -37,7 +37,8 @@  __novmx__libc_siglongjmp (__novmx__sigjmp_buf env, int val)
 
   if (env[0].__mask_was_saved)
     /* Restore the saved signal mask.  */
-    (void) __sigprocmask (SIG_SETMASK, &env[0].__saved_mask,
+    (void) __sigprocmask (SIG_SETMASK,
+			  (sigset_t *) &env[0].__saved_mask,
 			  (sigset_t *) NULL);
 
   /* Call the machine-dependent function to restore machine state.  */
diff --git a/sysdeps/powerpc/novmx-sigjmp.c b/sysdeps/powerpc/novmx-sigjmp.c
index 7d0ae59437..62680fe38f 100644
--- a/sysdeps/powerpc/novmx-sigjmp.c
+++ b/sysdeps/powerpc/novmx-sigjmp.c
@@ -35,7 +35,7 @@  __novmx__sigjmp_save (__novmx__sigjmp_buf env, int savemask)
 {
   env[0].__mask_was_saved = (savemask &&
 			     __sigprocmask (SIG_BLOCK, (sigset_t *) NULL,
-					    &env[0].__saved_mask) == 0);
+					    (sigset_t *) &env[0].__saved_mask) == 0);
 
   return 0;
 }
diff --git a/sysdeps/powerpc/sigjmp.c b/sysdeps/powerpc/sigjmp.c
index 6d593a0992..736491eff9 100644
--- a/sysdeps/powerpc/sigjmp.c
+++ b/sysdeps/powerpc/sigjmp.c
@@ -31,7 +31,7 @@  __vmx__sigjmp_save (sigjmp_buf env, int savemask)
 {
   env[0].__mask_was_saved = (savemask &&
 			     __sigprocmask (SIG_BLOCK, (sigset_t *) NULL,
-					    &env[0].__saved_mask) == 0);
+					    (sigset_t *) &env[0].__saved_mask) == 0);
 
   return 0;
 }
diff --git a/sysdeps/unix/sysv/linux/bits/setjmp3.h b/sysdeps/unix/sysv/linux/bits/setjmp3.h
new file mode 100644
index 0000000000..e86dbdcba4
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/bits/setjmp3.h
@@ -0,0 +1,48 @@ 
+/* __jmpbuf_target_t defition for Linux.
+   Copyright (C) 2017 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
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef _SETJMP_H
+# error "Never include <bits/setjmp3.h> directly; use <setjmp.h> instead."
+#endif
+
+#include <bits/types/__sigset_t.h>
+
+/* The biggest signal number + 1  */
+#define _JUMP_BUF_SIGSET_NSIG	257
+/* Number of longs to hold all signals.  */
+#define _JUMP_BUF_SIGSET_NWORDS \
+  ((_JUMP_BUF_SIGSET_NSIG - 1 + 7) / (8 * sizeof (unsigned long int)))
+
+typedef struct
+  {
+    unsigned long int __val[_JUMP_BUF_SIGSET_NWORDS];
+  } __jmp_buf_sigset_t;
+
+typedef union
+  {
+    __sigset_t __saved_mask_compat;
+    struct
+      {
+	__jmp_buf_sigset_t __saved_mask;
+	/* Paddings for target specific usage.  */
+	unsigned long int __padding[12];
+      } __saved;
+  } __jmpbuf_target_t;
+
+/* Saved signal mask.  */
+#define __saved_mask __target.__saved.__saved_mask
diff --git a/sysdeps/unix/sysv/linux/ia64/unwind_longjmp.c b/sysdeps/unix/sysv/linux/ia64/unwind_longjmp.c
index 874ed18d6b..658a25b87b 100644
--- a/sysdeps/unix/sysv/linux/ia64/unwind_longjmp.c
+++ b/sysdeps/unix/sysv/linux/ia64/unwind_longjmp.c
@@ -34,7 +34,8 @@  __libc_unwind_longjmp (sigjmp_buf env, int val)
 
   if (env[0].__mask_was_saved)
     /* Restore the saved signal mask.  */
-    (void) __sigprocmask (SIG_SETMASK, &env[0].__saved_mask,
+    (void) __sigprocmask (SIG_SETMASK,
+			  (sigset_t *) &env[0].__saved_mask,
 			  (sigset_t *) NULL);
 
   /* Call the machine-dependent function to restore machine state.  */