Revert Intel CET changes to __jmp_buf_tag (Bug 22743)
Commit Message
On 01/24/2018 05:48 PM, Dmitry V. Levin wrote:
> I'm afraid by Monday it will be too late for 2.27 as we will get very
> little testing before the release.
Before reverting:
[carlos@athas tst-cleanup1]$ /home/carlos/build/glibc/elf/ld.so --library-path /home/carlos/build/glibc:/home/carlos/build/glibc/elf:/home/carlos/build/glibc/dlfcn:/home/carlos/build/glibc/nptl ./tst-cleanup1
ch (3)
ch (2)
ch (1)
Didn't expect signal from child: got `Segmentation fault'
After reverting:
[carlos@athas tst-cleanup1]$ /home/carlos/build/glibc-reverted/elf/ld.so --library-path /home/carlos/build/glibc-reverted:/home/carlos/build/glibc-reverted/elf:/home/carlos/build/glibc-reverted/dlfcn:/home/carlos/build/glibc-reverted/nptl ./tst-cleanup1
ch (3)
ch (2)
ch (1)
~~~ Commit message ~~~
In commit cba595c350e52194e10c0006732e1991e3d0803b and commit
f81ddabffd76ac9dd600b02adbf3e1dac4bb10ec, ABI compatibility with
applications was broken by increasing the size of the on-stack
allocated __pthread_unwind_buf_t beyond the oringal size.
Applications only have the origianl space available for
__pthread_unwind_register, and __pthread_unwind_next to use,
any increase in the size of __pthread_unwind_buf_t causes these
functions to write beyond the original structure into other
on-stack variables leading to segmentation faults in common
applications like vlc. The only workaround is to version those
functions which operate on the old sized objects, but this must
happen in glibc 2.28.
Thank you to Andrew Senkevich, H.J. Lu, and Aurelien Jarno, for
submitting reports and tracking the issue down.
The commit reverts the above mentioned commits and testing on
x86_64 shows that the ABI compatibility is restored. A tst-cleanup1
regression test linked with an older glibc now passes when run
with the newly built glibc. Previously a tst-cleanup1 linked with
an older glibc would segfault when run with an affected glibc build.
Tested on x86_64 with no regressions.
Signed-off-by: Carlos O'Donell <carlos@redhat.com>
~~~
Patch attached.
OK to commit?
This fixes the last blocker for glibc 2.27.
Comments
On Wed, Jan 24, 2018 at 8:53 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 01/24/2018 05:48 PM, Dmitry V. Levin wrote:
>> I'm afraid by Monday it will be too late for 2.27 as we will get very
>> little testing before the release.
> Before reverting:
>
> [carlos@athas tst-cleanup1]$ /home/carlos/build/glibc/elf/ld.so --library-path /home/carlos/build/glibc:/home/carlos/build/glibc/elf:/home/carlos/build/glibc/dlfcn:/home/carlos/build/glibc/nptl ./tst-cleanup1
> ch (3)
> ch (2)
> ch (1)
> Didn't expect signal from child: got `Segmentation fault'
>
> After reverting:
>
> [carlos@athas tst-cleanup1]$ /home/carlos/build/glibc-reverted/elf/ld.so --library-path /home/carlos/build/glibc-reverted:/home/carlos/build/glibc-reverted/elf:/home/carlos/build/glibc-reverted/dlfcn:/home/carlos/build/glibc-reverted/nptl ./tst-cleanup1
> ch (3)
> ch (2)
> ch (1)
>
> ~~~ Commit message ~~~
> In commit cba595c350e52194e10c0006732e1991e3d0803b and commit
> f81ddabffd76ac9dd600b02adbf3e1dac4bb10ec, ABI compatibility with
> applications was broken by increasing the size of the on-stack
> allocated __pthread_unwind_buf_t beyond the oringal size.
> Applications only have the origianl space available for
> __pthread_unwind_register, and __pthread_unwind_next to use,
> any increase in the size of __pthread_unwind_buf_t causes these
> functions to write beyond the original structure into other
> on-stack variables leading to segmentation faults in common
> applications like vlc. The only workaround is to version those
> functions which operate on the old sized objects, but this must
> happen in glibc 2.28.
>
> Thank you to Andrew Senkevich, H.J. Lu, and Aurelien Jarno, for
> submitting reports and tracking the issue down.
>
> The commit reverts the above mentioned commits and testing on
> x86_64 shows that the ABI compatibility is restored. A tst-cleanup1
> regression test linked with an older glibc now passes when run
> with the newly built glibc. Previously a tst-cleanup1 linked with
> an older glibc would segfault when run with an affected glibc build.
>
> Tested on x86_64 with no regressions.
>
> Signed-off-by: Carlos O'Donell <carlos@redhat.com>
> ~~~
>
> Patch attached.
>
> OK to commit?
>
> This fixes the last blocker for glibc 2.27.
Please don't revert my patch. Please try this patch:
https://sourceware.org/git/?p=glibc.git;a=commit;h=4b7fc470a6740808b41502d7431f91805e272d26
instead. I will clean it up and submit it tomorrow.
Thanks.
On 01/25/2018 06:33 AM, H.J. Lu wrote:
> Please don't revert my patch. Please try this patch:
>
> https://sourceware.org/git/?p=glibc.git;a=commit;h=4b7fc470a6740808b41502d7431f91805e272d26
>
> instead. I will clean it up and submit it tomorrow.
I don't see how adding a symbol version to pthread_create helps to solve
the general case. Callers of pthread_register_cancel and pthread_create
are often compiled at different times. Not everyone does a mass rebuild
each time they switch to a new glibc version.
I still think you are over-engineering this. The pad array has still an
unused member (the last one). Just change sigsetjmp to store the shadow
pointer in that location, then the old and new setjmp will work with the
current stack layout. As far as I can tell, there are only 64 signals,
so you don't even have to change the location of the signal mask.
Furthermore, nothing in the toolchain prevents people from compiling
CET-marked binaries with older glibc headers, so you can't use CET
markup to determine the size of the stack allocation anyway.
Thanks,
Florian
On 01/24/2018 09:33 PM, H.J. Lu wrote:
> On Wed, Jan 24, 2018 at 8:53 PM, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 01/24/2018 05:48 PM, Dmitry V. Levin wrote:
>>> I'm afraid by Monday it will be too late for 2.27 as we will get very
>>> little testing before the release.
>> Before reverting:
>>
>> [carlos@athas tst-cleanup1]$ /home/carlos/build/glibc/elf/ld.so --library-path /home/carlos/build/glibc:/home/carlos/build/glibc/elf:/home/carlos/build/glibc/dlfcn:/home/carlos/build/glibc/nptl ./tst-cleanup1
>> ch (3)
>> ch (2)
>> ch (1)
>> Didn't expect signal from child: got `Segmentation fault'
>>
>> After reverting:
>>
>> [carlos@athas tst-cleanup1]$ /home/carlos/build/glibc-reverted/elf/ld.so --library-path /home/carlos/build/glibc-reverted:/home/carlos/build/glibc-reverted/elf:/home/carlos/build/glibc-reverted/dlfcn:/home/carlos/build/glibc-reverted/nptl ./tst-cleanup1
>> ch (3)
>> ch (2)
>> ch (1)
>>
>> ~~~ Commit message ~~~
>> In commit cba595c350e52194e10c0006732e1991e3d0803b and commit
>> f81ddabffd76ac9dd600b02adbf3e1dac4bb10ec, ABI compatibility with
>> applications was broken by increasing the size of the on-stack
>> allocated __pthread_unwind_buf_t beyond the oringal size.
>> Applications only have the origianl space available for
>> __pthread_unwind_register, and __pthread_unwind_next to use,
>> any increase in the size of __pthread_unwind_buf_t causes these
>> functions to write beyond the original structure into other
>> on-stack variables leading to segmentation faults in common
>> applications like vlc. The only workaround is to version those
>> functions which operate on the old sized objects, but this must
>> happen in glibc 2.28.
>>
>> Thank you to Andrew Senkevich, H.J. Lu, and Aurelien Jarno, for
>> submitting reports and tracking the issue down.
>>
>> The commit reverts the above mentioned commits and testing on
>> x86_64 shows that the ABI compatibility is restored. A tst-cleanup1
>> regression test linked with an older glibc now passes when run
>> with the newly built glibc. Previously a tst-cleanup1 linked with
>> an older glibc would segfault when run with an affected glibc build.
>>
>> Tested on x86_64 with no regressions.
>>
>> Signed-off-by: Carlos O'Donell <carlos@redhat.com>
>> ~~~
>>
>> Patch attached.
>>
>> OK to commit?
>>
>> This fixes the last blocker for glibc 2.27.
>
> Please don't revert my patch. Please try this patch:
>
> https://sourceware.org/git/?p=glibc.git;a=commit;h=4b7fc470a6740808b41502d7431f91805e272d26
>
> instead. I will clean it up and submit it tomorrow.
This is unacceptable. It adds a new symbol version and we froze the
ABI at the start of the month. You cannot work these fixes into 2.27
with a new symbol version, it must wait for 2.28.
On 01/25/2018 05:37 PM, Carlos O'Donell wrote:
>> Please don't revert my patch. Please try this patch:
>>
>> https://sourceware.org/git/?p=glibc.git;a=commit;h=4b7fc470a6740808b41502d7431f91805e272d26
>>
>> instead. I will clean it up and submit it tomorrow.
> This is unacceptable. It adds a new symbol version and we froze the
> ABI at the start of the month. You cannot work these fixes into 2.27
> with a new symbol version, it must wait for 2.28.
Note that the actually posted patch is completely different.
Thanks,
Florian
From 440f414842b61923dc8219b26df10d2a12de0f82 Mon Sep 17 00:00:00 2001
From: Carlos O'Donell <carlos@systemhalted.org>
Date: Wed, 24 Jan 2018 20:35:22 -0800
Subject: [PATCH] Revert Intel CET changes to __jmp_buf_tag (Bug 22743)
In commit cba595c350e52194e10c0006732e1991e3d0803b and commit
f81ddabffd76ac9dd600b02adbf3e1dac4bb10ec, ABI compatibility with
applications was broken by increasing the size of the on-stack
allocated __pthread_unwind_buf_t beyond the oringal size.
Applications only have the origianl space available for
__pthread_unwind_register, and __pthread_unwind_next to use,
any increase in the size of __pthread_unwind_buf_t causes these
functions to write beyond the original structure into other
on-stack variables leading to segmentation faults in common
applications like vlc. The only workaround is to version those
functions which operate on the old sized objects, but this must
happen in glibc 2.28.
Thank you to Andrew Senkevich, H.J. Lu, and Aurelien Jarno, for
submitting reports and tracking the issue down.
The commit reverts the above mentioned commits and testing on
x86_64 shows that the ABI compatibility is restored. A tst-cleanup1
regression test linked with an older glibc now passes when run
with the newly built glibc. Previously a tst-cleanup1 linked with
an older glibc would segfault when run with an affected glibc build.
Tested on x86_64 with no regressions.
Signed-off-by: Carlos O'Donell <carlos@redhat.com>
---
ChangeLog | 30 ++++++++++++++++++
bits/types/__cancel_jmp_buf_tag.h | 28 -----------------
nptl/Makefile | 3 +-
nptl/descr.h | 3 --
sysdeps/i386/nptl/tcb-offsets.sym | 1 -
sysdeps/i386/nptl/tls.h | 4 ---
sysdeps/nptl/pthread.h | 7 +++--
sysdeps/unix/sysv/linux/hppa/pthread.h | 7 +++--
.../linux/x86/bits/types/__cancel_jmp_buf_tag.h | 31 -------------------
sysdeps/unix/sysv/linux/x86/nptl/pthreadP.h | 36 ----------------------
sysdeps/unix/sysv/linux/x86/pthreaddef.h | 22 -------------
sysdeps/x86_64/nptl/tcb-offsets.sym | 1 -
sysdeps/x86_64/nptl/tls.h | 5 +--
13 files changed, 42 insertions(+), 136 deletions(-)
delete mode 100644 bits/types/__cancel_jmp_buf_tag.h
delete mode 100644 sysdeps/unix/sysv/linux/x86/bits/types/__cancel_jmp_buf_tag.h
delete mode 100644 sysdeps/unix/sysv/linux/x86/nptl/pthreadP.h
delete mode 100644 sysdeps/unix/sysv/linux/x86/pthreaddef.h
@@ -1,3 +1,33 @@
+2018-01-24 Carlos O'Donll <carlos@redhat.com>
+
+ Revert:
+
+ 2017-12-19 H.J. Lu <hongjiu.lu@intel.com>
+
+ [BZ #22563]
+ * sysdeps/i386/nptl/tcb-offsets.sym (FEATURE_1_OFFSET): New.
+ * sysdeps/i386/nptl/tls.h (tcbhead_t): Add feature_1.
+ * sysdeps/x86_64/nptl/tcb-offsets.sym (FEATURE_1_OFFSET): New.
+ * sysdeps/x86_64/nptl/tls.h (tcbhead_t): Rename __glibc_unused1
+ to feature_1.
+
+ 2017-12-19 H.J. Lu <hongjiu.lu@intel.com>
+
+ [BZ #22563]
+ * bits/types/__cancel_jmp_buf_tag.h: New file.
+ * sysdeps/unix/sysv/linux/x86/bits/types/__cancel_jmp_buf_tag.h
+ * sysdeps/unix/sysv/linux/x86/pthreaddef.h: Likewise.
+ * sysdeps/unix/sysv/linux/x86/nptl/pthreadP.h: Likewise.
+ * nptl/Makefile (headers): Add
+ bits/types/__cancel_jmp_buf_tag.h.
+ * nptl/descr.h [NEED_SAVED_MASK_IN_CANCEL_JMP_BUF]
+ (pthread_unwind_buf): Add saved_mask to cancel_jmp_buf.
+ * sysdeps/nptl/pthread.h: Include
+ <bits/types/__cancel_jmp_buf_tag.h>.
+ (__pthread_unwind_buf_t): Use struct __cancel_jmp_buf_tag with
+ __cancel_jmp_buf.
+ * sysdeps/unix/sysv/linux/hppa/pthread.h: Likewise.
+
2018-01-24 Joseph Myers <joseph@codesourcery.com>
* sysdeps/unix/sysv/linux/m68k/jmp_buf-macros.h: Move to ....
deleted file mode 100644
@@ -1,28 +0,0 @@
-/* Define struct __cancel_jmp_buf_tag.
- Copyright (C) 2017-2018 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 ____cancel_jmp_buf_tag_defined
-#define ____cancel_jmp_buf_tag_defined 1
-
-struct __cancel_jmp_buf_tag
- {
- __jmp_buf __cancel_jmp_buf;
- int __mask_was_saved;
- };
-
-#endif
@@ -22,8 +22,7 @@ subdir := nptl
include ../Makeconfig
-headers := pthread.h semaphore.h bits/semaphore.h \
- bits/types/__cancel_jmp_buf_tag.h
+headers := pthread.h semaphore.h bits/semaphore.h
extra-libs := libpthread
extra-libs-others := $(extra-libs)
@@ -65,9 +65,6 @@ struct pthread_unwind_buf
{
__jmp_buf jmp_buf;
int mask_was_saved;
-#ifdef NEED_SAVED_MASK_IN_CANCEL_JMP_BUF
- __sigset_t saved_mask;
-#endif
} cancel_jmp_buf[1];
union
@@ -15,4 +15,3 @@ POINTER_GUARD offsetof (tcbhead_t, pointer_guard)
#ifndef __ASSUME_PRIVATE_FUTEX
PRIVATE_FUTEX offsetof (tcbhead_t, private_futex)
#endif
-FEATURE_1_OFFSET offsetof (tcbhead_t, feature_1)
@@ -50,10 +50,6 @@ typedef struct
void *__private_tm[4];
/* GCC split stack support. */
void *__private_ss;
- /* Bit 0: IBT.
- Bit 1: SHSTK.
- */
- unsigned int feature_1;
} tcbhead_t;
# define TLS_MULTIPLE_THREADS_IN_TCB 1
@@ -27,7 +27,6 @@
#include <bits/setjmp.h>
#include <bits/wordsize.h>
#include <bits/types/struct_timespec.h>
-#include <bits/types/__cancel_jmp_buf_tag.h>
/* Detach state. */
@@ -524,7 +523,11 @@ extern void pthread_testcancel (void);
typedef struct
{
- struct __cancel_jmp_buf_tag __cancel_jmp_buf[1];
+ struct
+ {
+ __jmp_buf __cancel_jmp_buf;
+ int __mask_was_saved;
+ } __cancel_jmp_buf[1];
void *__pad[4];
} __pthread_unwind_buf_t __attribute__ ((__aligned__));
@@ -27,7 +27,6 @@
#include <bits/setjmp.h>
#include <bits/wordsize.h>
#include <bits/types/struct_timespec.h>
-#include <bits/types/__cancel_jmp_buf_tag.h>
/* Detach state. */
@@ -500,7 +499,11 @@ extern void pthread_testcancel (void);
typedef struct
{
- struct __cancel_jmp_buf_tag __cancel_jmp_buf[1];
+ struct
+ {
+ __jmp_buf __cancel_jmp_buf;
+ int __mask_was_saved;
+ } __cancel_jmp_buf[1];
void *__pad[4];
} __pthread_unwind_buf_t __attribute__ ((__aligned__));
deleted file mode 100644
@@ -1,31 +0,0 @@
-/* Define struct __cancel_jmp_buf_tag.
- Copyright (C) 2017-2018 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 ____cancel_jmp_buf_tag_defined
-#define ____cancel_jmp_buf_tag_defined 1
-
-#include <bits/types/__sigset_t.h>
-
-struct __cancel_jmp_buf_tag
- {
- __jmp_buf __cancel_jmp_buf;
- int __mask_was_saved;
- __sigset_t __saved_mask;
- };
-
-#endif
deleted file mode 100644
@@ -1,36 +0,0 @@
-/* Internal pthread header. Linux/x86 version.
- Copyright (C) 2017-2018 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_next <nptl/pthreadP.h>
-
-#ifndef _PTHREADP_H_X86
-#define _PTHREADP_H_X86 1
-
-extern struct pthread_unwind_buf ____pthread_unwind_buf_private;
-
-_Static_assert (sizeof (____pthread_unwind_buf_private.cancel_jmp_buf)
- >= sizeof (struct __jmp_buf_tag),
- "size of cancel_jmp_buf < sizeof __jmp_buf_tag");
-
-extern __pthread_unwind_buf_t ____pthread_unwind_buf;
-
-_Static_assert (sizeof (____pthread_unwind_buf.__cancel_jmp_buf)
- >= sizeof (struct __jmp_buf_tag),
- "size of __cancel_jmp_buf < sizeof __jmp_buf_tag");
-
-#endif
deleted file mode 100644
@@ -1,22 +0,0 @@
-/* Pthread macros. Linux/x86 version.
- Copyright (C) 2017-2018 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_next <pthreaddef.h>
-
-/* Need saved_mask in cancel_jmp_buf. */
-#define NEED_SAVED_MASK_IN_CANCEL_JMP_BUF 1
@@ -15,7 +15,6 @@ VGETCPU_CACHE_OFFSET offsetof (tcbhead_t, vgetcpu_cache)
#ifndef __ASSUME_PRIVATE_FUTEX
PRIVATE_FUTEX offsetof (tcbhead_t, private_futex)
#endif
-FEATURE_1_OFFSET offsetof (tcbhead_t, feature_1)
-- Not strictly offsets, but these values are also used in the TCB.
TCB_CANCELSTATE_BITMASK CANCELSTATE_BITMASK
@@ -56,10 +56,7 @@ typedef struct
# else
int __glibc_reserved1;
# endif
- /* Bit 0: IBT.
- Bit 1: SHSTK.
- */
- unsigned int feature_1;
+ int __glibc_unused1;
/* Reservation of some values for the TM ABI. */
void *__private_tm[4];
/* GCC split stack support. */
--
2.14.3