-
-
Notifications
You must be signed in to change notification settings - Fork 31.7k
Insufficient sigaltstack size used by CPython prevents extensions from using new ISA #91124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
The following snippet illustrates request by an extension to use AMX registers:
This request fails on the account of too small a size for sigaltstack used by CPython allocated in Modules/faulthandler.c The stack size used is 2*SIGSTKSZ, and does not take hardware capabilities into account. Linux kernel 5.14 adds support to query minimum size of sigaltstack dynamically via getauxval(AT_MINSIGSTKSZ). AMX support is added in Linux kernel 5.16 CPython should make use of this when built against more recent Linux kernels. |
The latest faulthandler bug on sigaltstack() was bpo-21131 about FPU state stored by "XSAVE" and a Linux kernel change to fix a security vulnerability: |
Thanks Oleksandr Pavlyk for your nice enhancement! I merged your change to 3.9, 3.10 and main branches! I also added os.sysconf('SC_MINSIGSTKSZ') to Python 3.11. |
On my x86-64 Fedora 35 with the CPU "Intel(R) Core(TM) i7-6820HQ CPU @ 2.70GHz", I get:
In C, sysconf(_SC_MINSIGSTKSZ) is similar to getauxval(AT_MINSIGSTKSZ), but the glibc sysconf(_SC_MINSIGSTKSZ) is more generic, whereas IMO getauxval(AT_MINSIGSTKSZ) is more the low-level Linux API. |
So where getauxval(AT_MINSIGSTKSZ) < SIGSTKSZ the merged changes actually resulted in decrease of the allocated signal deliver stack. On Sapphire Rapids due to tile registers size we have getauxval(AT_MINSIGSTKSZ) > SIGSTKSZ. This is why the initial proposal was to use SIGSTKSZ + max(getauxval(AT_MINSIGSTKSZ), SIGSTKSZ). I suppose, ultimately I am saying that we should check that bpo-21131 does not regress. |
faulthandler stack is only used in the least likely case: on a fatal error. It should reduce its memory footprint, so it's good that it uses less memory. To fix bpo-21131, I chose to use SIGSTKSZ * 2 because:
Previously, faulthandler had between 0 and SIGSTKSZ bytes depending on which CPU extension was used or not on the process. If I understood correctly, on Linux 5.14 with the change, faulthandler now always has SIGSTKSZ bytes for its own usage, and the other bytes are used by the Linux kernel. So it's more reliable for faulthandler, to always have the same amount of memory for its personal use. |
Commit 393e2bf has broken CPython in RedHat 6: [2022-03-16T18:49:20.608Z] 2022/03/16 14:48:55 ERROR /tmp/python3.10-3.10.3-0/Modules/faulthandler.c:28:12: fatal error: sys/auxv.h: No such file or directory |
This is problematic because this has been backported to stable releases. |
We may need to revert these commits and do another release.... sigh :( |
The configure file is checking for "linux/auxvec.h" checking linux/auxvec.h usability... yes but the code is including "sys/auxvec.h" |
The code is assuming that if linux/auxvec.h then sys/auxv.h will be available, which is wrong. |
This may be quite bad, because this means that 3.10 and 3.9 doesn't build in CentOS 6, which is used for manylinux2010 wheels |
Too bad that we want to support RHEL 6 but have no buildbot for that. |
Thanks for the fix Pablo. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: