test_faulthandler.test_register_chain fails on 64bit ppc/arm with kernel >= 3.10

[bkabrda]

BPO	21131
Nosy	@vstinner, @miss-islington, @peadar, @justbennet
PRs	bpo-21131: don't use SIGSTKSZ for stack size in sigaltstack #13649 bpo-21131: Fix faulthandler.register(chain=True) stack #15276 [3.8] bpo-21131: Fix faulthandler.register(chain=True) stack (GH-15276) #15291 [3.7] bpo-21131: Fix faulthandler.register(chain=True) stack (GH-15276) #15292
Files	test_register_chain_segfault_reproducer.py: Minimal segfault reproducer sigaltstack-stacksize.patch: tentative fix for issue altstack.c

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-08-14.22:15:57.928>
created_at = <Date 2014-04-02.09:17:18.405>
labels = ['3.8', '3.7', 'tests', '3.9']
title = 'test_faulthandler.test_register_chain fails on 64bit ppc/arm with kernel >= 3.10'
updated_at = <Date 2019-08-19.10:13:40.764>
user = 'https://bugs.python.org/bkabrda'

bugs.python.org fields:

activity = <Date 2019-08-19.10:13:40.764>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2019-08-14.22:15:57.928>
closer = 'vstinner'
components = ['Tests']
creation = <Date 2014-04-02.09:17:18.405>
creator = 'bkabrda'
dependencies = []
files = ['34699', '48353', '48542']
hgrepos = []
issue_num = 21131
keywords = ['patch']
message_count = 33.0
messages = ['215365', '215368', '215370', '215371', '215373', '278053', '278112', '343405', '343767', '343769', '349604', '349625', '349681', '349686', '349687', '349688', '349692', '349694', '349695', '349698', '349710', '349715', '349755', '349756', '349757', '349764', '349766', '349767', '349768', '349796', '349797', '349799', '349949']
nosy_count = 7.0
nosy_names = ['vstinner', 'bkabrda', 'opoplawski', 'miss-islington', 'markmcclain', 'peadar', 'justbennet']
pr_nums = ['13649', '15276', '15291', '15292']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue21131'
versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']

[bkabrda]

test_faulthandler.test_register_chain fails on some 64bit architectures (arm8, ppc64) with kernel >= 3.10:

======================================================================
FAIL: test_register_chain (main.FaultHandlerTests)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/builddir/build/BUILD/Python-3.3.2/Lib/test/test_faulthandler.py", line 588, in test_register_chain
    self.check_register(chain=True)
  File "/builddir/build/BUILD/Python-3.3.2/Lib/test/test_faulthandler.py", line 572, in check_register
    self.assertEqual(exitcode, 0)
AssertionError: -11 != 0

I created a minimal reproducer (reproduces the issue on 3.3, 3.4 and dev) of this segfault (attached). When run with GC assertions turned on, Python fails with:

python: /builddir/build/BUILD/Python-3.3.2/Modules/gcmodule.c:332: update_refs: Assertion `gc->gc.gc_refs == (-3)\' failed.

We experienced this issue when building Python 3.3 on Fedora's arm8 builder [1], it seems that opensuse have experienced the same failure on ppc64 [2] and ubuntu has a similar issue in their 64bit arm builds [3].

It seems that this may be caused by a change in kernel, since it's only reproducible on kernel >= 3.10. A nice explanation of what goes on and how the problem can be narrowed down is at the opensuse bug report [4], this is basically where I got stuck with this problem, too.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1045193
[2] https://bugzilla.novell.com/show_bug.cgi?id=831629
[3] https://bugs.launchpad.net/ubuntu/+source/python3.4/+bug/1264354
[4] https://bugzilla.novell.com/show_bug.cgi?id=831629#c15

[vstinner]

"test_faulthandler.test_register_chain fails on some 64bit architectures (arm8, ppc64) with kernel >= 3.10"

I am a little bit surprised that the bug depends on the kernel version.

Does test_register_chain_segfault_reproducer.py also crash with chain=False?

Can you check if HAVE_SIGACTION is defined in pyconfig.h? Or in Python: import sysconfig; print(sysconfig.get_config_var('HAVE_SIGACTION')).

Wit chain=True, faulthandler_register() registers its signal handler with SA_NODEFER flag:
---

    /* if the signal is received while the kernel is executing a system
       call, try to restart the system call instead of interrupting it and
       return EINTR. */
    action.sa_flags = SA_RESTART;
    if (chain) {
        /* do not prevent the signal from being received from within its
           own signal handler */
        action.sa_flags = SA_NODEFER;
    }

---

With chain=True, the faulthandler_user() function calls the previous signal handler with:
---

#ifdef HAVE_SIGACTION
    if (user->chain) {
        (void)sigaction(signum, &user->previous, NULL);
        errno = save_errno;

    /* call the previous signal handler */
    raise(signum);

        save_errno = errno;
        (void)faulthandler_register(signum, user->chain, NULL);
        errno = save_errno;
    }
#else
    if (user->chain) {
        errno = save_errno;
        /* call the previous signal handler */
        user->previous(signum);
    }
#endif

---

You can try to add "#undef HAVE_SIGACTION" in faulthandler.c (after #include "Python.h") to see if the bug can be reproduced without sigaction. The code using signal() is very different, especially when chaining signal handlers.

[vstinner]

Would it be possible to list the kernel version (full version including the minor version) on which the test works or crash? You are talking about "3.10" without the minor version. It may be a regression in the kernel.

Example of change in Linux kernel 3.10.17:
---
commit 19a420033da02200c424adfa3a7b9eed6e3a6dc2
Author: Christian Ruppert <christian.ruppert@abilis.com>
Date: Wed Oct 2 11:13:38 2013 +0200

ARC: Fix signal frame management for SA_SIGINFO

commit 10469350e345599dfef3fa78a7c19fb230e674c1 upstream.

Previously, when a signal was registered with SA_SIGINFO, parameters 2
and 3 of the signal handler were written to registers r1 and r2 before
the register set was saved. This led to corruption of these two
registers after returning from the signal handler (the wrong values were
restored).
With this patch, registers are now saved before any parameters are
passed, thus maintaining the processor state from before signal entry.

Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---

Extract of changes of Linux 3.10.6:
---
ARM: move signal handlers into a vdso-like page
ARM: make vectors page inaccessible from userspace
ARM: fix a cockup in 48be69a02 (ARM: move signal handlers into a vdso-like page)
ARM: fix nommu builds with 48be69a02 (ARM: move signal handlers into a vdso-like page)
---
Commit:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=48be69a02

I would like to make sure that the bug is not a kernel bug.

[bkabrda]

I'm also surprised that this depends on kernel version, however that's what I found out (and the opensuse guys seem to only have reproduced this on kernel >= 3.10, too).

- Full kernel version (uname -r output): 3.13.0-0.rc7.28.sa2.aarch64
- The reproducer *doesn't* crash with chain=False.
- HAVE_SIGACTION:
>>> import sysconfig; print(sysconfig.get_config_var('HAVE_SIGACTION'))
1
- I'll do rebuild with "#undef HAVE_SIGACTION" and post my results here as soon as it's finished.

[bkabrda]

Ok, so with "#undef HAVE_SIGACTION" both the reproducer and the original test (as well as all tests in test_faulthandler) pass fine.

[opoplawski]

We're running into this building python 3.4.3 on EL6 ppc64. The os kernel is 4.7.2-201.fc24.ppc64, but the EL6 chroot kernel-headers are 2.6.32-642.4.2.el6. Any progress here?

[vstinner]

We're running into this building python 3.4.3 on EL6 ppc64. The os kernel is 4.7.2-201.fc24.ppc64, but the EL6 chroot kernel-headers are 2.6.32-642.4.2.el6. Any progress here?

Sorry, but if I'm unable to reproduce the issue, I cannot make progress on analyzing the bug. I would need an remote access (SSH) to a computer where the bug can be reproduced.

I also would like to know if the issue can be reproduced in C.

faulthandler depends a lot how signals and threads are implemented. I'm not completely suprised to see bugs on some specific platforms.

[peadar]

Hi - we ran into what looks like exactly this issue on an x86_64 sporadically, and tracked down the root cause.

When faulthandler.c uses sigaltstack(2), the stack size is set up with a buffer of size SIGSTKSZ. That is, sadly, only 8k.

When a signal is raised, before the handler is called, the kernel stores the machine state on the user's (possibly "alternate") stack. The size of that state is very much variable, depending on the CPU.

When we chain the signal handler in the sigaction variant of the code in faulthandler, we raise the signal with the existing handler still on the stack, and save a second copy of the CPU state.

Finally, when any part of that signal handler has to invoke a function that requires the dynamic linker's intervention to resolve, it will call some form of _dl_runtime_resolve* - likely _dl_runtime_resolve_xsave or _dl_runtime_resolve_xsavec.

These functions will also have to save machine state. So, how big is the machine state? Well, it depends on the CPU.
On one machine I have access to, /proc/cpuinfo shows "Intel(R) Xeon(R) CPU E5-2640 v4", I have:

(gdb) p _rtld_local_ro._dl_x86_cpu_features.xsave_state_size
$1 = 896

On another machine, reporting as "Intel(R) Xeon(R) Gold 5118 CPU", I have:

(gdb) p _rtld_local_ro._dl_x86_cpu_features.xsave_state_size
$1 = 2560

This means that the required stack space to hold 3 sets of CPU state is over 7.5k. And, for the signal handlers, it's actually worse: more like 3.25k per frame. A chained signal handler that needs to invoke dynamic linking will therefore consume more than the default stack space allocated in faulthandler.c, just in machine-state saves alone. So, the failing test is failing because its scribbling on random memory before the allocated stack space.

My guess is that the previous architectures this manifested in have larger stack demands for signal handling than x86_64, but clearly newer x86_64 processors are starting to get tickled by this.

Fix is pretty simple - just allocate more stack space. The attached patch uses pthread_attr_getstacksize to find the system's default stack size, and then uses that as the default, and also defines an absolute minimum stack size of 1M. This fixes the issue on our machine with the big xsave state size. (I'm sure I'm getting the feature test macros wrong for testing for pthreads availability)

Also, I think in the case of a threaded environment, using the altstack might not be the best choice - I think multiple threads handling signals that run on that stack will wind up stomping on the same memory - is there a strong reason to maintain this altstack behaviour?

[vstinner]

+ pthread_attr_t attrs;
+ pthread_attr_init(&attrs);
+ (void)pthread_attr_getstacksize(&attrs, &stack.ss_size);

PyThread_start_new_thread() of thread_pthread.h already contains logic to get a "good" stack size. I would prefer to reuse this code.

See also _pythread_nt_set_stacksize() of thread_nt.h.

Maybe we need a private function to get the default stack size?

See also PyThread_get_stacksize() and _thread.stack_size().

[peadar]

Ok - let me submit a pull request with your suggestions

On Tue, 28 May 2019 at 13:08, STINNER Victor <report@bugs.python.org> wrote:

STINNER Victor <vstinner@redhat.com> added the comment:

• pthread_attr_t attrs;
• pthread_attr_init(&attrs);
• (void)pthread_attr_getstacksize(&attrs, &stack.ss_size);

PyThread_start_new_thread() of thread_pthread.h already contains logic to
get a "good" stack size. I would prefer to reuse this code.

See also _pythread_nt_set_stacksize() of thread_nt.h.

Maybe we need a private function to get the default stack size?

See also PyThread_get_stacksize() and _thread.stack_size().

----------

---

Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue21131\>

---

[justbennet]

One additional note on this. Thanks to a colleague at USC who pointed out that this bug does not seem to get exercised if one does not include --enable-shared at configuration.

I confirmed this using the distributed Python-3.7.4.tgz file and configure --prefix=/path/to/install. The make test then runs to completion with no errors. Running make distclean, then rerunning with configure --prefix=/path/to/install --enable-shared will then exercise the bug, and the faulthandler test fails with a segfault.

Applying the patch file attached to this report and rebuilding leads to make test passing all tests, also.

If someone was trying to reproduce and did not configure shared libraries, then that would have failed to reproduce.

[justbennet]

Perhaps I should add, that we are able to reproduce this behavior on this hardware

Dell R640 Intel(R) Xeon(R) Gold 6140 CPU @ 2.30GHz
Dell R740 Intel(R) Xeon(R) Gold 6130 CPU @ 2.10GHz
Dell R440 Intel(R) Xeon(R) Silver 4116 CPU @ 2.10GHz
Dell C6420 Intel(R) Xeon(R) Silver 4116 CPU @ 2.10GHz

but not on other chips, such as a recent i5, Haswell family, and older. That may also make a difference if someone tries to reproduce this.

[vstinner]

I dislike PR 13649 because it touch the thread module, only to fix a faulthandler unit test. The relationship between thread stack size and faulthandler is not obvious to me. Currently, faulthandler uses SIGSTKSZ, not the thread stack size. faulthandler usage of the stack should be quite low: it should need less than 1 MiB for example.

"When faulthandler.c uses sigaltstack(2), the stack size is set up with a buffer of size SIGSTKSZ. That is, sadly, only 8k."

"A chained signal handler that needs to invoke dynamic linking will therefore consume more than the default stack space allocated in faulthandler.c, just in machine-state saves alone. So, the failing test is failing because its scribbling on random memory before the allocated stack space."

Aha, that's interesting: SIGSTKSZ should be enough for 1 signal handler, but test_register_chain calls 2 signal handlers using the same stack. Can you please try the following patch?

diff --git a/Modules/faulthandler.c b/Modules/faulthandler.c
index 2331051f79..e7d13f2b2d 100644
--- a/Modules/faulthandler.c
+++ b/Modules/faulthandler.c
@@ -1325,7 +1325,7 @@ _PyFaulthandler_Init(int enable)
      * be able to allocate memory on the stack, even on a stack overflow. If it
      * fails, ignore the error. */
     stack.ss_flags = 0;
-    stack.ss_size = SIGSTKSZ;
+    stack.ss_size = SIGSTKSZ * 2;
     stack.ss_sp = PyMem_Malloc(stack.ss_size);
     if (stack.ss_sp != NULL) {
         err = sigaltstack(&stack, &old_stack);

[vstinner]

Attached altstack.c mimicks faulthandler unit test test_register_chain(), except that faulthandler_user() uses almost no stack memory. This test should check if SIGSTKSZ is big enough to call a second signal handler from a first signal handler.

Example of my Fedora 30 with "Intel(R) Core(TM) i7-6820HQ CPU @ 2.70GHz" model name in /proc/cpuinfo and Linux kernel 5.1.18-300.fc30.x86_64:

$ gcc altstack.c -o altstack && ./altstack
SIGSTKSZ = 8192
our signal handler
User defined signal 1

Note: the follow gdb command didn't work for me:

(gdb) p _rtld_local_ro._dl_x86_cpu_features.xsave_state_size
$1 = 896

How can I get xsave_state_size for my glibc/kernel/CPU?