uretprobe: change syscall number, again

RFC In review Arnd Bergmann 4 participants 5 messages Jul 30, 2024 – Aug 2, 2024

Subsystems (7)

x86 Kselftest BPF RISC-V +3 more subsystems

lore

Share & tools

lore

Arnd Bergmann

author

2024-07-30 15:45 UTC #0 lore raw

RFC 1/1 uretprobe: change syscall number, again

From: Arnd Bergmann <[email protected]>

Despite multiple attempts to get the syscall number assignment right for the newly added uretprobe syscall, we ended up with a bit of a mess:

- The number is defined as 467 based on the assumption that the xattrat family of syscalls would use 463 through 466, but those did not make it into 6.11.

- The include/uapi/asm-generic/unistd.h file still lists the number 463, but the new scripts/syscall.tbl that was supposed to have the same data lists 467 instead as the number for arc, arm64, csky, hexagon, loongarch, nios2, openrisc and riscv. None of these architectures actually provide a uretprobe syscall.

- All the other architectures (powerpc, arm, mips, ...) don't list this syscall at all.

There are two ways to make it consistent again: either list it with the same syscall number on all architectures, or only list it on x86 but not in scripts/syscall.tbl and asm-generic/unistd.h.

Based on the most recent discussion, it seems like we won't need it anywhere else, so just remove the inconsistent assignment and instead move the x86 number to the next available one in the architecture specific range, which is 335.

Fixes 5c28424e9a34 ("syscalls: Fix to add sys_uretprobe to syscall.tbl")

Fixes 190fec72df4a ("uprobe: Wire up uretprobe system call")

Fixes 63ded110979b ("uprobe: Change uretprobe syscall scope and number")

Cc Linus Torvalds <[email protected]>

Signed-off-by Arnd Bergmann <[email protected]>

---
I think we should fix this as soon as possible. Please let me know if you agree on this approach, or prefer one of the alternatives.

I've queued up this version in the asm-generic tree so I can send a pull request in the next few days, but I'm fine with doing this a differently if someone has a stronger opinion on what numbers to assign for it on earch architecture.

 arch/x86/entry/syscalls/syscall_64.tbl | 2 +-
 include/uapi/asm-generic/unistd.h      | 5 +----
 scripts/syscall.tbl                    | 1 -
 3 files changed, 2 insertions(+), 6 deletions(-)

arch/x86/entry/syscalls/syscall_64.tbl +1 -1

index 83073fa3c989..7093ee21c0d1 100644
@@ -344,6 +344,7 @@
 332	common	statx			sys_statx
 333	common	io_pgetevents		sys_io_pgetevents
 334	common	rseq			sys_rseq
+335	common	uretprobe		sys_uretprobe
 # don't use numbers 387 through 423, add new calls after the last
 # 'common' entry
 424	common	pidfd_send_signal	sys_pidfd_send_signal

@@ -385,7 +386,6 @@
 460	common	lsm_set_self_attr	sys_lsm_set_self_attr
 461	common	lsm_list_modules	sys_lsm_list_modules
 462 	common  mseal			sys_mseal
-467	common	uretprobe		sys_uretprobe
 
 #
 # Due to a historical design error, certain syscalls are numbered differently

include/uapi/asm-generic/unistd.h +1 -4

index 985a262d0f9e..5bf6148cac2b 100644
@@ -841,11 +841,8 @@ __SYSCALL(__NR_lsm_list_modules, sys_lsm_list_modules)
 #define __NR_mseal 462
 __SYSCALL(__NR_mseal, sys_mseal)
 
-#define __NR_uretprobe 463
-__SYSCALL(__NR_uretprobe, sys_uretprobe)
-
 #undef __NR_syscalls
-#define __NR_syscalls 464
+#define __NR_syscalls 463
 
 /*
  * 32 bit systems traditionally used different

scripts/syscall.tbl +0 -1

index 591d85e8ca7e..797e20ea99a2 100644
@@ -402,4 +402,3 @@
 460	common	lsm_set_self_attr		sys_lsm_set_self_attr
 461	common	lsm_list_modules		sys_lsm_list_modules
 462	common	mseal				sys_mseal
-467	common	uretprobe			sys_uretprobe

Signature · 1 line

2.39.2

Masami Hiramatsu (Google)

Acked-by on v1 1/1

2024-08-02 09:14 UTC #1 lore raw

Quoted 8 lines · On Tue, 30 Jul 2024 17:43:36 +0200 Arnd Bergmann <[email protected]> wrote:

From: Arnd Bergmann <[email protected]>
Despite multiple attempts to get the syscall number assignment right for the newly added uretprobe syscall, we ended up with a bit of a mess:
- The number is defined as 467 based on the assumption that the xattrat family of syscalls would use 463 through 466, but those did not make it into 6.11.

OK... that was not expected.

Quoted 6 lines

- The include/uapi/asm-generic/unistd.h file still lists the number 463, but the new scripts/syscall.tbl that was supposed to have the same data lists 467 instead as the number for arc, arm64, csky, hexagon, loongarch, nios2, openrisc and riscv. None of these architectures actually provide a uretprobe syscall.

Oops, thanks for finding.

Quoted 3 lines

- All the other architectures (powerpc, arm, mips, ...) don't list this syscall at all.

OK, so even if it is not supported on those, we need to put it as a placeholder.

Quoted 10 lines

There are two ways to make it consistent again: either list it with the same syscall number on all architectures, or only list it on x86 but not in scripts/syscall.tbl and asm-generic/unistd.h.
Based on the most recent discussion, it seems like we won't need it anywhere else, so just remove the inconsistent assignment and instead move the x86 number to the next available one in the architecture specific range, which is 335.

Quoted 8 lines

Fixes 5c28424e9a34 ("syscalls: Fix to add sys_uretprobe to syscall.tbl")
Fixes 190fec72df4a ("uprobe: Wire up uretprobe system call")
Fixes 63ded110979b ("uprobe: Change uretprobe syscall scope and number")
Cc Linus Torvalds <[email protected]>
Signed-off-by Arnd Bergmann <[email protected]>
---
I think we should fix this as soon as possible. Please let me know if you agree on this approach, or prefer one of the alternatives.

OK, I think it is good. But you missed to fix a selftest code which also needs to be updated.

Could you revert commit 3e301b431b91 ("selftests/bpf: Change uretprobe
syscall number in uprobe_syscall test") too?

Acked-by Masami Hiramatsu (Google) <[email protected]>

Thank you,

Quoted 60 lines

 arch/x86/entry/syscalls/syscall_64.tbl | 2 +-
 include/uapi/asm-generic/unistd.h      | 5 +----
 scripts/syscall.tbl                    | 1 -
 3 files changed, 2 insertions(+), 6 deletions(-)

arch/x86/entry/syscalls/syscall_64.tbl +1 -1

index 83073fa3c989..7093ee21c0d1 100644
@@ -344,6 +344,7 @@
 332	common	statx			sys_statx
 333	common	io_pgetevents		sys_io_pgetevents
 334	common	rseq			sys_rseq
+335	common	uretprobe		sys_uretprobe
 # don't use numbers 387 through 423, add new calls after the last
 # 'common' entry
 424	common	pidfd_send_signal	sys_pidfd_send_signal

@@ -385,7 +386,6 @@
 460	common	lsm_set_self_attr	sys_lsm_set_self_attr
 461	common	lsm_list_modules	sys_lsm_list_modules
 462 	common  mseal			sys_mseal
-467	common	uretprobe		sys_uretprobe
 
 #
 # Due to a historical design error, certain syscalls are numbered differently

include/uapi/asm-generic/unistd.h +1 -4

index 985a262d0f9e..5bf6148cac2b 100644
@@ -841,11 +841,8 @@ __SYSCALL(__NR_lsm_list_modules, sys_lsm_list_modules)
 #define __NR_mseal 462
 __SYSCALL(__NR_mseal, sys_mseal)
 
-#define __NR_uretprobe 463
-__SYSCALL(__NR_uretprobe, sys_uretprobe)
-
 #undef __NR_syscalls
-#define __NR_syscalls 464
+#define __NR_syscalls 463
 
 /*
  * 32 bit systems traditionally used different

scripts/syscall.tbl +0 -1

index 591d85e8ca7e..797e20ea99a2 100644
@@ -402,4 +402,3 @@
 460	common	lsm_set_self_attr		sys_lsm_set_self_attr
 461	common	lsm_list_modules		sys_lsm_list_modules
 462	common	mseal				sys_mseal
-467	common	uretprobe			sys_uretprobe

Signature · 1 line

2.39.2

Signature · 1 line

Masami Hiramatsu (Google) <[email protected]>

Jiri Olsa

Reviewed-by

2024-08-02 11:52 UTC #2 lore raw

Quoted 52 lines · Masami Hiramatsu

Quoted 8 lines · On Tue, 30 Jul 2024 17:43:36 +0200 Arnd Bergmann <[email protected]> wrote:
From: Arnd Bergmann <[email protected]>
Despite multiple attempts to get the syscall number assignment right for the newly added uretprobe syscall, we ended up with a bit of a mess:
- The number is defined as 467 based on the assumption that the xattrat family of syscalls would use 463 through 466, but those did not make it into 6.11.
OK... that was not expected.
Quoted 6 lines
- The include/uapi/asm-generic/unistd.h file still lists the number 463, but the new scripts/syscall.tbl that was supposed to have the same data lists 467 instead as the number for arc, arm64, csky, hexagon, loongarch, nios2, openrisc and riscv. None of these architectures actually provide a uretprobe syscall.
Oops, thanks for finding.
Quoted 3 lines
- All the other architectures (powerpc, arm, mips, ...) don't list this syscall at all.
OK, so even if it is not supported on those, we need to put it as a placeholder.
Quoted 10 lines
There are two ways to make it consistent again: either list it with the same syscall number on all architectures, or only list it on x86 but not in scripts/syscall.tbl and asm-generic/unistd.h.
Based on the most recent discussion, it seems like we won't need it anywhere else, so just remove the inconsistent assignment and instead move the x86 number to the next available one in the architecture specific range, which is 335.
Quoted 8 lines
Fixes 5c28424e9a34 ("syscalls: Fix to add sys_uretprobe to syscall.tbl")
Fixes 190fec72df4a ("uprobe: Wire up uretprobe system call")
Fixes 63ded110979b ("uprobe: Change uretprobe syscall scope and number")
Cc Linus Torvalds <[email protected]>
Signed-off-by Arnd Bergmann <[email protected]>
---
I think we should fix this as soon as possible. Please let me know if you agree on this approach, or prefer one of the alternatives.
OK, I think it is good. But you missed to fix a selftest code which also needs to be updated.

Quoted 6 lines

Could you revert commit 3e301b431b91 ("selftests/bpf: Change uretprobe
syscall number in uprobe_syscall test") too?
Acked-by Masami Hiramatsu (Google) <[email protected]>
Thank you,

yes, it still needs the selftest change like below otherwise if that new number works for you then lgtm

Reviewed-by Jiri Olsa <[email protected]>

thanks,
jirka

---

tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c +1 -1

index bd8c75b620c2..5f78edca6540 100644
@@ -216,7 +216,7 @@ static void test_uretprobe_regs_change(void)
 }
 
 #ifndef __NR_uretprobe
-#define __NR_uretprobe 467
+#define __NR_uretprobe 335
 #endif
 
 __naked unsigned long uretprobe_syscall_call_1(void)

Arnd Bergmann

maintainer

2024-08-02 13:21 UTC #3 lore raw

In reply to Masami Hiramatsu (Google)

Quoted 10 lines · Masami Hiramatsu

Quoted 3 lines · On Tue, 30 Jul 2024 17:43:36 +0200 Arnd Bergmann <[email protected]> wrote:
---
I think we should fix this as soon as possible. Please let me know if you agree on this approach, or prefer one of the alternatives.
OK, I think it is good. But you missed to fix a selftest code which also needs to be updated.
Could you revert commit 3e301b431b91 ("selftests/bpf: Change uretprobe
syscall number in uprobe_syscall test") too?

I folded the change that Jiri suggested now.

Quoted 1 line

Acked-by Masami Hiramatsu (Google) <[email protected]>

Thanks,

Arnd

Masami Hiramatsu (Google)

2024-08-02 14:00 UTC #4 lore raw

Quoted 6 lines · On Fri, 02 Aug 2024 15:18:52 +0200 "Arnd Bergmann" <[email protected]> wrote:

Quoted 5 lines · Masami Hiramatsu
Quoted 3 lines · On Tue, 30 Jul 2024 17:43:36 +0200 Arnd Bergmann <[email protected]> wrote:
---
I think we should fix this as soon as possible. Please let me know if you agree on this approach, or prefer one of the alternatives.

Quoted 7 lines

Quoted 5 lines
OK, I think it is good. But you missed to fix a selftest code which also needs to be updated.
Could you revert commit 3e301b431b91 ("selftests/bpf: Change uretprobe
syscall number in uprobe_syscall test") too?
I folded the change that Jiri suggested now.

Thanks Jiri and Arnd for fix that!

Quoted 7 lines

Quoted 2 lines
Acked-by Masami Hiramatsu (Google) <[email protected]>
Thanks,
Arnd

Signature · 1 line

Masami Hiramatsu (Google) <[email protected]>