-
Notifications
You must be signed in to change notification settings - Fork 173
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
RFE: Sync to Linux 6.12 syscall definitions #435
Conversation
aecd5e9
to
f36e149
Compare
f36e149
to
f4afd16
Compare
After the change, |
this is really helpful for LoongArch, thanks. |
Hi maintainers,
Please review. |
Could you review this PR? Thanks. |
https://github.com/seccomp/libseccomp?tab=readme-ov-file#release-process TL;DR: When it's ready. |
#else | ||
#define __SNR_map_shadow_stack __PNR_map_shadow_stack | ||
#endif | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't map_shadow_stack()
defined for all the arches?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is, but in this commit the PNR was allocated anyway and the change is already shipped in v2.5.5. So if we're to consider PNR values as part of the public API we would have to keep __PNR_map_shadow_stack
defined, and for stylistic consistency we would have to keep this #ifdef
-fery as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reminder @xen0n.
In the manpages we "highly recommend" the use of the SCMP_SYS()
macro as opposed to directly specifying syscall numbers so it's a bit of a gray area. I think it would be okay for us to modify/remove the PNR syscall values if necessary, but we probably shouldn't do so unless we have a good reason. Considering everything, it looks like you approach of leaving the PNR values as-is is likely the right one. Thanks :)
Just one small comment/clarification/question above, but otherwise I think this looks good. Thanks for all your work on this @xen0n! |
include/seccomp-syscalls.h
Outdated
@@ -280,6 +280,9 @@ | |||
#define __PNR_atomic_barrier -10246 | |||
#define __PNR_atomic_cmpxchg_32 -10247 | |||
#define __PNR_getpagesize -10248 | |||
#define __PNR_map_shadow_stack -10249 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FTR: The PNR numbers below -10244
(__PNR_memfd_secret) have already diverged between libseccomp:main
and libseccomp:release-2.5
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a FYI, I'm okay with fixing that sort of thing while merging the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have preserved the __PNR_map_shadow_stack
position while rebasing.
Apart from de-duplication of logic, this refactor is also going to help syncing to the Linux 6.11+ definitions, where all architectures are converted to source their syscall definitions from syscall.tbl files. The change is tested on Linux 6.2 sources to not affect the generated syscalls.csv apart from timestamp changes. Signed-off-by: WANG Xuerui <git@xen0n.name>
The aarch64, loongarch64 and riscv64 architectures have their syscall table sources changed to scripts/syscall.tbl, from the original inclusion of asm-generic/unistd.h. Make the script recognize the new format for these architectures. Signed-off-by: WANG Xuerui <git@xen0n.name>
Due to the addition of fstat & newfstatat to the LoongArch syscall ABI, tests 38 and 55 have to be updated for the changed syscall numbers. As for the PNR additions, normally they are allocated alphabetically for the syscalls introduced between updates of the table, but in the v2.5 release branch -10245 is already assigned to map_shadow_stack in commit 53267af ("all: update the syscall table for Linux v6.7-rc3"). While the map_shadow_stack syscall is in fact available across all architectures, for consistency with v2.5.5 and later it is kept in the same position in this update. Signed-off-by: WANG Xuerui <git@xen0n.name>
f4afd16
to
d909801
Compare
This is now rebased and updated to reflect Linux v6.12-rc5 syscall info (no new syscalls since 6.11). I have also incorporated @rusty-snake's review comment of keeping PNR allocations consistent with v2.5.x. Tests still pass on aarch64, loongarch64, riscv64 and x86_64. |
My apologies for being quiet the last few weeks. I've had some other high-priority work on my plate, but I'm thinking I should have some time in the next few weeks to get back into libseccomp work. Thanks all for your patience :). |
This looked good to me and it passed all my tests locally so I merged the PR at HEAD f01e675, thanks for you work and patience @xen0n! @drakenclimber nothing in here looked controversial and we needed something like this for future syscall updates so I went ahead and merged the PR, if you notice anything awry please let me know and we can revert/fix it. |
While the PR is merged, I'm going to leave this open until I finish the backport to the release-2.5 branch. |
Here is the release-2.5/v2.5.x branch backport: #439 |
With the backport PR now posted, I'm closing this PR, thanks again everyone! |
The
arch-syscall-validate
script updates will need to be backported to v2.5.x later, for continuing to sync aarch64 and riscv64 that got refactored to source the syscall definitions from asyscall.tbl
instead ofasm-generic/unistd.h
.