[issue21131] test_faulthandler.test_register_chain fails on 64bit ppc/arm with kernel >= 3.10
Peter Edwards added the comment:
Hi Victor, thanks for the comments. Responses inline below.
On Wed, 14 Aug 2019 at 12:25, STINNER Victor wrote:
>
> STINNER Victor added the comment:
>
> I dislike PR 13649 because it touch the thread module, only to fix a
> faulthandler unit test.
My original patch (posted in the comments above) was purely in faulthandler
- I went at the threading code on your suggestion:
PyThread_start_new_thread() of thread_pthread.h already contains logic to
> get a "good" stack size. I would prefer to reuse this code."
I have no problem reformulating the code to avoid touching the threads
library - let me redo it as such.
> 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.
>
The point of contention here is really he choice of stack size. SIGSTKSZ is
ridiculously small - it is the bare minimum amount of memory required to
actually handle the signal. The signal handling mechanism eats a huge chunk
of it, and the dynamic linker can also eat several K too. The intent was to
use the default thread stack size as a heuristic for what the platform
considers to be a reasonable size stack for applications. If the
pthread-aware OS is somehow constrained for address space, then I'd expect
it to reflect that in the default stack size. For 32-bit linux, the 8M of
address space is a bit of a chunk, but it's not a huge proportion of the
3-4G you have, and you're not consuming actual memory. On a 64-bit system,
consuming 8M of address space is a drop in the ocean.
>
> "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?
>
It's more complex than that - in dynamically linked applications when you
call functions that still need to be resolved by the dynamic linker, the
resolving thunk in the PLT also ends up saving the register state via
xsavec, so with a chained call, there are up to 3 register states saved on
the stack, each over 2.5k on actual hardware we have now. I'm not convinced
there are not other ways stack space will be consumed during the signal
handler, and I'm not convinced that the amount of memory required per
handler will not go up as new CPUs come out, and I'm not convinced that
SIGSTKSZ will be bumped to reflect that (it certainly hasn't in the past),
so scaling SIGSTKSZ like this, while it'll likely fix the problem on any
machine I can test it on, doesn't seem like a stable solution
> 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);
>
> --
>
> ___
> Python tracker
> <https://bugs.python.org/issue21131>
> ___
>
--
___
Python tracker
<https://bugs.python.org/issue21131>
___
___
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21131] test_faulthandler.test_register_chain fails on 64bit ppc/arm with kernel >= 3.10
Peter Edwards added the comment: The patch I originally proposed here ( https://bugs.python.org/file48353/sigaltstack-stacksize.patch ) is a pretty minimal fix that uses the pthread stack size where available, with a hard-coded lower bound of 1M. @Victor : if you want a minimal diff, I can drop the existing PR, submit the above as a new one, and we can progress from there? Let me know how you'd like me to proceed. On Wed, 14 Aug 2019 at 14:26, Bennet Fauber wrote: > > Bennet Fauber added the comment: > > I just tested the proposed change in > > 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 > . . . . > -stack.ss_size = SIGSTKSZ; > +stack.ss_size = SIGSTKSZ * 2; > ``` > > and the segfault no longer occurs at the faulthandler test. > > Compiling and running the altstack.c using the system installed GCC 4.8.5 > on CentOS 7.6.1810, kernel version 3.10.0-957.10.1.el7.x86_64 running on > Dell R640 Intel(R) Xeon(R) Gold 6140 CPU @ 2.30GHz results in this output. > > ``` > $ gcc -o altstack altstack.c > $ ./altstack > SIGSTKSZ = 8192 > our signal handler > User defined signal 1 > ``` > > It does seem to me that relying on a statically set stack size when using > dynamically loaded libraries is inviting similar problems in the future for > the reasons that Peter enumerated: There is no telling now what the > requirements will be for some new chip family, and one cannot predict now > what additional (if any) memory requirements might be needed by the linker > in the future. > > But, I think getting _some_ patch accepted and pushed to the main Python > releases should have some priority, as the current state does seem > undesirable. > > -- > > ___ > Python tracker > <https://bugs.python.org/issue21131> > ___ > -- ___ Python tracker <https://bugs.python.org/issue21131> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21131] test_faulthandler.test_register_chain fails on 64bit ppc/arm with kernel >= 3.10
Peter Edwards added the comment: On Wed, 14 Aug 2019 at 14:46, STINNER Victor wrote: > > STINNER Victor added the comment: > > "On a 64-bit system, consuming 8M of address space is a drop in the ocean." > > Let me disagree here. Python always allocates faulthandler stack, even if > faulthandler is not used. Even when faulthandler is used, I would prefer to > not waste memory if 8 KiB is good enough. > I can understand the aversion to the waste when its never used - I can address 37851 if you like - it seems pretty simple to fix. The pedant in me must point out that it's 8M of address space, not memory. The cost on 64-bit (well, with a 47-bit user address space) is vanishingly small, regardless of the physical memory on the system. On 32-bit, it's 0.2% of your address space, which I think I'd trade for the safety, but that's your call, and I appreciate that address space can be a constrained resource on 32-bit systems. I do think SIGSTKSZ*2=16k is far too small considering the fault handler could be running arbitrary python code, and we know that there's somewhat less than half of that available for use by the interpreter. > > By the way, I just created bpo-37851 to allocate this stack at the first > faulthandler usage, instead of always allocating it, even when faulthandler > is not used. > > I wrote PR 15276 to use a stack of SIGSTKSZ*2 bytes. According to > msg349694, it does fix the crash. > > Can someone please double check that PR 15276 fix test_faulthandler on a > platform where the test crash without this change? > I can confirm that on the specific hardware I could reproduce this, that PR14276 and setting the stacksize to SIGSTKSZ*2 passes the test_faulthandler test. > > -- > > ___ > Python tracker > <https://bugs.python.org/issue21131> > ___ > -- ___ Python tracker <https://bugs.python.org/issue21131> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21131] test_faulthandler.test_register_chain fails on 64bit ppc/arm with kernel >= 3.10
Peter Edwards added the comment: On Wed, 14 Aug 2019 at 22:34, STINNER Victor wrote: > > ...I'm not sure that we can fix bpo-37851 in Python 3.7. That's totally reasonable, sure. -- ___ Python tracker <https://bugs.python.org/issue21131> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21131] test_faulthandler.test_register_chain fails on 64bit ppc/arm with kernel >= 3.10
Peter Edwards added the comment: On Wed, 14 Aug 2019 at 22:32, STINNER Victor wrote: > > We are talking abou the faulthandler_user() function of > Modules/faulthandler.c. It is implemented in pure C, it doesn't allocate > memory on the heap, it uses a very small set of functions (write(), > sigaction(), raise()) and it tries to minimize its usage of the stack > memory. > I was more concerned about what was happening in the chained handler, which will also run on the restricted stack: I had assumed that was potentially running arbitrary python code. That's actually probably incorrect, now that I think about it, but it's harder to infer much about its stack usage directly in faulthandler.c. I'll take a look (just to satisfy myself, more than anything) > It is very different than the traceback module which is implemented in > pure Python. > Right, totally - I had jumped to the conclusion that it would end up executing in the interpreter via the chain, but, as I say, that's probably wrong. I'm not sure what guarantees the chained signal handler makes about its stack usage. (Will educate myself) > faulthandler is really designed to debug segmentation fault, stack > overflow, Python hang (like a deadlock), etc. > -- > > ___ > Python tracker > <https://bugs.python.org/issue21131> > ___ > -- ___ Python tracker <https://bugs.python.org/issue21131> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21131] test_faulthandler.test_register_chain fails on 64bit ppc/arm with kernel >= 3.10
Peter Edwards added the comment: On Wed, 14 Aug 2019 at 23:13, STINNER Victor wrote: > > STINNER Victor added the comment: > > About PR 13649, I'm not sure that _PyThread_preferred_stacksize() is still > relevant, since my change fixed test_faulthandler test_register_chain(). I > chose my change since it's less invasive: it only impacts faulthandler, and > it minimalizes the memory usage (especially when faulthandler is not used). > Sure - there's no reason for it to exist if you don't want to use it to fix the issue here. > Python/thread_pthread.h refactor changes of PR 13649 are interested. Would > you like to extract them into a new PR which doesn't add > _PyThread_preferred_stacksize() but just add new PLATFORM_xxx macros? > Yes, certainly. Maybe test_faulthandler will fail tomorrow on a new platform, but I prefer > to open a discussion once such case happens, rather than guessing how > faulthandler can crash on an hypothetical platforms. Well, one argument for the dynamic approach is that existing python binaries can adjust without needing to be respun for new CPUs. I think SIGSTKSZ is a vestage from when CPU architectures had consistently sized register sets across models. Its interesting to read the comment on the IA64 definition for SIGSTKSZ: https://github.com/torvalds/linux/blob/master/arch/ia64/include/uapi/asm/signal.h#L83 > I'm sure that libc developers are well aware of the FPU state size and > update SIGSTKSZ accordingly. > The current value comes from the kernel sources, and has not changed since at the latest 2005 (with the initial git commit of the kernel), which I think predates xsave/xrestore by some margin. I don't think its a useful measure of anything in the real (x86) world today. > glibc code computing xsave_state_size: > > > https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/x86/cpu-features.c;h=4bab1549132fe8a4c203a70b8c7a51c1dc304049;hb=HEAD#l223 > > -- > > If tomorrow, it becomes too hard to choose a good default value for > faulthandler stack size, another workaround would be to make it > configurable, as Python lets developers choose the thread stack size: > _thread.stack_size(size). > > -- > > ___ > Python tracker > <https://bugs.python.org/issue21131> > ___ > -- ___ Python tracker <https://bugs.python.org/issue21131> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21131] test_faulthandler.test_register_chain fails on 64bit ppc/arm with kernel >= 3.10
Peter Edwards added the comment: 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? -- keywords: +patch nosy: +peadar Added file: https://bugs.python.org/file48353/sigaltstack-stacksize.patch ___ Python tracker <https://bugs.python.org/issue21131> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21131] test_faulthandler.test_register_chain fails on 64bit ppc/arm with kernel >= 3.10
Peter Edwards added the comment: Ok - let me submit a pull request with your suggestions On Tue, 28 May 2019 at 13:08, STINNER Victor wrote: > > STINNER Victor 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 > <https://bugs.python.org/issue21131> > ___ > -- ___ Python tracker <https://bugs.python.org/issue21131> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue21131] test_faulthandler.test_register_chain fails on 64bit ppc/arm with kernel >= 3.10
Change by Peter Edwards : -- pull_requests: +13543 stage: -> patch review pull_request: https://github.com/python/cpython/pull/13649 ___ Python tracker <https://bugs.python.org/issue21131> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue35484] Segmentation fault due to faulthandler on Solaris
Peter Edwards added the comment: This is likely a duplicate of issue 21131 -- nosy: +peadar ___ Python tracker <https://bugs.python.org/issue35484> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
