MIPS n32 is one of two ILP32 architectures supported by the kernel that have 64-bit syscall arguments (another one is x32).
When this test passed 32-bit arguments to syscall(), they were sign-extended in libc, PTRACE_GET_SYSCALL_INFO reported these sign-extended 64-bit values, and the test complained about the mismatch.
Fix this by passing arguments of the appropriate type to syscall(), which is "unsigned long long" on MIPS n32, and __kernel_ulong_t on other architectures.
As a side effect, this also extends the test on all 64-bit architectures by choosing constants that don't fit into 32-bit integers.
Signed-off-by: Dmitry V. Levin ldv@strace.io ---
v2: Fixed MIPS #ifdef.
.../selftests/ptrace/get_syscall_info.c | 53 +++++++++++-------- 1 file changed, 32 insertions(+), 21 deletions(-)
diff --git a/tools/testing/selftests/ptrace/get_syscall_info.c b/tools/testing/selftests/ptrace/get_syscall_info.c index 5bcd1c7b5be6..2970f72d66d3 100644 --- a/tools/testing/selftests/ptrace/get_syscall_info.c +++ b/tools/testing/selftests/ptrace/get_syscall_info.c @@ -11,8 +11,19 @@ #include <err.h> #include <signal.h> #include <asm/unistd.h> +#include <linux/types.h> #include "linux/ptrace.h"
+#if defined(_MIPS_SIM) && _MIPS_SIM == _MIPS_SIM_NABI32 +/* + * MIPS N32 is the only architecture where __kernel_ulong_t + * does not match the bitness of syscall arguments. + */ +typedef unsigned long long kernel_ulong_t; +#else +typedef __kernel_ulong_t kernel_ulong_t; +#endif + static int kill_tracee(pid_t pid) { @@ -42,37 +53,37 @@ sys_ptrace(int request, pid_t pid, unsigned long addr, unsigned long data)
TEST(get_syscall_info) { - static const unsigned long args[][7] = { + const kernel_ulong_t args[][7] = { /* a sequence of architecture-agnostic syscalls */ { __NR_chdir, - (unsigned long) "", - 0xbad1fed1, - 0xbad2fed2, - 0xbad3fed3, - 0xbad4fed4, - 0xbad5fed5 + (uintptr_t) "", + (kernel_ulong_t) 0xdad1bef1bad1fed1ULL, + (kernel_ulong_t) 0xdad2bef2bad2fed2ULL, + (kernel_ulong_t) 0xdad3bef3bad3fed3ULL, + (kernel_ulong_t) 0xdad4bef4bad4fed4ULL, + (kernel_ulong_t) 0xdad5bef5bad5fed5ULL }, { __NR_gettid, - 0xcaf0bea0, - 0xcaf1bea1, - 0xcaf2bea2, - 0xcaf3bea3, - 0xcaf4bea4, - 0xcaf5bea5 + (kernel_ulong_t) 0xdad0bef0caf0bea0ULL, + (kernel_ulong_t) 0xdad1bef1caf1bea1ULL, + (kernel_ulong_t) 0xdad2bef2caf2bea2ULL, + (kernel_ulong_t) 0xdad3bef3caf3bea3ULL, + (kernel_ulong_t) 0xdad4bef4caf4bea4ULL, + (kernel_ulong_t) 0xdad5bef5caf5bea5ULL }, { __NR_exit_group, 0, - 0xfac1c0d1, - 0xfac2c0d2, - 0xfac3c0d3, - 0xfac4c0d4, - 0xfac5c0d5 + (kernel_ulong_t) 0xdad1bef1fac1c0d1ULL, + (kernel_ulong_t) 0xdad2bef2fac2c0d2ULL, + (kernel_ulong_t) 0xdad3bef3fac3c0d3ULL, + (kernel_ulong_t) 0xdad4bef4fac4c0d4ULL, + (kernel_ulong_t) 0xdad5bef5fac5c0d5ULL } }; - const unsigned long *exp_args; + const kernel_ulong_t *exp_args;
pid_t pid = fork();
@@ -154,7 +165,7 @@ TEST(get_syscall_info) } ASSERT_LT(0, (rc = sys_ptrace(PTRACE_GET_SYSCALL_INFO, pid, size, - (unsigned long) &info))) { + (uintptr_t) &info))) { LOG_KILL_TRACEE("PTRACE_GET_SYSCALL_INFO: %m"); } ASSERT_EQ(expected_none_size, rc) { @@ -177,7 +188,7 @@ TEST(get_syscall_info) case SIGTRAP | 0x80: ASSERT_LT(0, (rc = sys_ptrace(PTRACE_GET_SYSCALL_INFO, pid, size, - (unsigned long) &info))) { + (uintptr_t) &info))) { LOG_KILL_TRACEE("PTRACE_GET_SYSCALL_INFO: %m"); } switch (ptrace_stop) {
Could somebody pick up this patch, please?
Nothing has changed since v2, so I have nothing new to add.
v2: https://lore.kernel.org/all/20250115233747.GA28541@strace.io/
On Thu, Jan 16, 2025 at 01:37:47AM +0200, Dmitry V. Levin wrote:
MIPS n32 is one of two ILP32 architectures supported by the kernel that have 64-bit syscall arguments (another one is x32).
When this test passed 32-bit arguments to syscall(), they were sign-extended in libc, PTRACE_GET_SYSCALL_INFO reported these sign-extended 64-bit values, and the test complained about the mismatch.
Fix this by passing arguments of the appropriate type to syscall(), which is "unsigned long long" on MIPS n32, and __kernel_ulong_t on other architectures.
As a side effect, this also extends the test on all 64-bit architectures by choosing constants that don't fit into 32-bit integers.
Signed-off-by: Dmitry V. Levin ldv@strace.io
v2: Fixed MIPS #ifdef.
.../selftests/ptrace/get_syscall_info.c | 53 +++++++++++-------- 1 file changed, 32 insertions(+), 21 deletions(-)
diff --git a/tools/testing/selftests/ptrace/get_syscall_info.c b/tools/testing/selftests/ptrace/get_syscall_info.c index 5bcd1c7b5be6..2970f72d66d3 100644 --- a/tools/testing/selftests/ptrace/get_syscall_info.c +++ b/tools/testing/selftests/ptrace/get_syscall_info.c @@ -11,8 +11,19 @@ #include <err.h> #include <signal.h> #include <asm/unistd.h> +#include <linux/types.h> #include "linux/ptrace.h" +#if defined(_MIPS_SIM) && _MIPS_SIM == _MIPS_SIM_NABI32 +/*
- MIPS N32 is the only architecture where __kernel_ulong_t
- does not match the bitness of syscall arguments.
- */
+typedef unsigned long long kernel_ulong_t; +#else +typedef __kernel_ulong_t kernel_ulong_t; +#endif
static int kill_tracee(pid_t pid) { @@ -42,37 +53,37 @@ sys_ptrace(int request, pid_t pid, unsigned long addr, unsigned long data) TEST(get_syscall_info) {
- static const unsigned long args[][7] = {
- const kernel_ulong_t args[][7] = { /* a sequence of architecture-agnostic syscalls */ { __NR_chdir,
(unsigned long) "",0xbad1fed1,0xbad2fed2,0xbad3fed3,0xbad4fed4,0xbad5fed5
(uintptr_t) "",(kernel_ulong_t) 0xdad1bef1bad1fed1ULL,(kernel_ulong_t) 0xdad2bef2bad2fed2ULL,(kernel_ulong_t) 0xdad3bef3bad3fed3ULL,(kernel_ulong_t) 0xdad4bef4bad4fed4ULL,(kernel_ulong_t) 0xdad5bef5bad5fed5ULL
0xcaf0bea0,0xcaf1bea1,0xcaf2bea2,0xcaf3bea3,0xcaf4bea4,0xcaf5bea5
(kernel_ulong_t) 0xdad0bef0caf0bea0ULL,(kernel_ulong_t) 0xdad1bef1caf1bea1ULL,(kernel_ulong_t) 0xdad2bef2caf2bea2ULL,(kernel_ulong_t) 0xdad3bef3caf3bea3ULL,(kernel_ulong_t) 0xdad4bef4caf4bea4ULL,(kernel_ulong_t) 0xdad5bef5caf5bea5ULL
0xfac1c0d1,0xfac2c0d2,0xfac3c0d3,0xfac4c0d4,0xfac5c0d5
(kernel_ulong_t) 0xdad1bef1fac1c0d1ULL,(kernel_ulong_t) 0xdad2bef2fac2c0d2ULL,(kernel_ulong_t) 0xdad3bef3fac3c0d3ULL,(kernel_ulong_t) 0xdad4bef4fac4c0d4ULL,(kernel_ulong_t) 0xdad5bef5fac5c0d5ULL
- const unsigned long *exp_args;
- const kernel_ulong_t *exp_args;
pid_t pid = fork(); @@ -154,7 +165,7 @@ TEST(get_syscall_info) } ASSERT_LT(0, (rc = sys_ptrace(PTRACE_GET_SYSCALL_INFO, pid, size,
(unsigned long) &info))) {
(uintptr_t) &info))) { LOG_KILL_TRACEE("PTRACE_GET_SYSCALL_INFO: %m"); } ASSERT_EQ(expected_none_size, rc) {@@ -177,7 +188,7 @@ TEST(get_syscall_info) case SIGTRAP | 0x80: ASSERT_LT(0, (rc = sys_ptrace(PTRACE_GET_SYSCALL_INFO, pid, size,
(unsigned long) &info))) {
(uintptr_t) &info))) { LOG_KILL_TRACEE("PTRACE_GET_SYSCALL_INFO: %m"); } switch (ptrace_stop) {-- ldv
On 1/15/25 16:37, Dmitry V. Levin wrote:
MIPS n32 is one of two ILP32 architectures supported by the kernel that have 64-bit syscall arguments (another one is x32).
When this test passed 32-bit arguments to syscall(), they were sign-extended in libc, PTRACE_GET_SYSCALL_INFO reported these sign-extended 64-bit values, and the test complained about the mismatch.
Fix this by passing arguments of the appropriate type to syscall(), which is "unsigned long long" on MIPS n32, and __kernel_ulong_t on other architectures.
As a side effect, this also extends the test on all 64-bit architectures by choosing constants that don't fit into 32-bit integers.
Signed-off-by: Dmitry V. Levin ldv@strace.io
v2: Fixed MIPS #ifdef.
.../selftests/ptrace/get_syscall_info.c | 53 +++++++++++-------- 1 file changed, 32 insertions(+), 21 deletions(-)
diff --git a/tools/testing/selftests/ptrace/get_syscall_info.c b/tools/testing/selftests/ptrace/get_syscall_info.c index 5bcd1c7b5be6..2970f72d66d3 100644 --- a/tools/testing/selftests/ptrace/get_syscall_info.c +++ b/tools/testing/selftests/ptrace/get_syscall_info.c @@ -11,8 +11,19 @@ #include <err.h> #include <signal.h> #include <asm/unistd.h> +#include <linux/types.h> #include "linux/ptrace.h" +#if defined(_MIPS_SIM) && _MIPS_SIM == _MIPS_SIM_NABI32 +/*
- MIPS N32 is the only architecture where __kernel_ulong_t
- does not match the bitness of syscall arguments.
- */
+typedef unsigned long long kernel_ulong_t; +#else +typedef __kernel_ulong_t kernel_ulong_t; +#endif
What's the reason for adding these typedefs? checkpatch should have warned you about adding new typedefs.
Also this introduces kernel_ulong_t in user-space test code. Something to avoid.
static int kill_tracee(pid_t pid) { @@ -42,37 +53,37 @@ sys_ptrace(int request, pid_t pid, unsigned long addr, unsigned long data) TEST(get_syscall_info) {
- static const unsigned long args[][7] = {
- const kernel_ulong_t args[][7] = { /* a sequence of architecture-agnostic syscalls */ { __NR_chdir,
(unsigned long) "",0xbad1fed1,0xbad2fed2,0xbad3fed3,0xbad4fed4,0xbad5fed5
(uintptr_t) "",
You could use ifdef here.
(kernel_ulong_t) 0xdad1bef1bad1fed1ULL,(kernel_ulong_t) 0xdad2bef2bad2fed2ULL,(kernel_ulong_t) 0xdad3bef3bad3fed3ULL,(kernel_ulong_t) 0xdad4bef4bad4fed4ULL,(kernel_ulong_t) 0xdad5bef5bad5fed5ULL
0xcaf0bea0,0xcaf1bea1,0xcaf2bea2,0xcaf3bea3,0xcaf4bea4,0xcaf5bea5
(kernel_ulong_t) 0xdad0bef0caf0bea0ULL,(kernel_ulong_t) 0xdad1bef1caf1bea1ULL,(kernel_ulong_t) 0xdad2bef2caf2bea2ULL,(kernel_ulong_t) 0xdad3bef3caf3bea3ULL,(kernel_ulong_t) 0xdad4bef4caf4bea4ULL,(kernel_ulong_t) 0xdad5bef5caf5bea5ULL
0xfac1c0d1,0xfac2c0d2,0xfac3c0d3,0xfac4c0d4,0xfac5c0d5
(kernel_ulong_t) 0xdad1bef1fac1c0d1ULL,(kernel_ulong_t) 0xdad2bef2fac2c0d2ULL,(kernel_ulong_t) 0xdad3bef3fac3c0d3ULL,(kernel_ulong_t) 0xdad4bef4fac4c0d4ULL,(kernel_ulong_t) 0xdad5bef5fac5c0d5ULL
- const unsigned long *exp_args;
- const kernel_ulong_t *exp_args;
pid_t pid = fork(); @@ -154,7 +165,7 @@ TEST(get_syscall_info) } ASSERT_LT(0, (rc = sys_ptrace(PTRACE_GET_SYSCALL_INFO, pid, size,
(unsigned long) &info))) {
(uintptr_t) &info))) { LOG_KILL_TRACEE("PTRACE_GET_SYSCALL_INFO: %m"); } ASSERT_EQ(expected_none_size, rc) {@@ -177,7 +188,7 @@ TEST(get_syscall_info) case SIGTRAP | 0x80: ASSERT_LT(0, (rc = sys_ptrace(PTRACE_GET_SYSCALL_INFO, pid, size,
(unsigned long) &info))) {
(uintptr_t) &info))) { LOG_KILL_TRACEE("PTRACE_GET_SYSCALL_INFO: %m"); } switch (ptrace_stop) {
thanks, -- Shuah
On Fri, Mar 28, 2025 at 05:04:54PM -0600, Shuah Khan wrote:
On 1/15/25 16:37, Dmitry V. Levin wrote:
MIPS n32 is one of two ILP32 architectures supported by the kernel that have 64-bit syscall arguments (another one is x32).
When this test passed 32-bit arguments to syscall(), they were sign-extended in libc, PTRACE_GET_SYSCALL_INFO reported these sign-extended 64-bit values, and the test complained about the mismatch.
Fix this by passing arguments of the appropriate type to syscall(), which is "unsigned long long" on MIPS n32, and __kernel_ulong_t on other architectures.
As a side effect, this also extends the test on all 64-bit architectures by choosing constants that don't fit into 32-bit integers.
Signed-off-by: Dmitry V. Levin ldv@strace.io
v2: Fixed MIPS #ifdef.
.../selftests/ptrace/get_syscall_info.c | 53 +++++++++++-------- 1 file changed, 32 insertions(+), 21 deletions(-)
diff --git a/tools/testing/selftests/ptrace/get_syscall_info.c b/tools/testing/selftests/ptrace/get_syscall_info.c index 5bcd1c7b5be6..2970f72d66d3 100644 --- a/tools/testing/selftests/ptrace/get_syscall_info.c +++ b/tools/testing/selftests/ptrace/get_syscall_info.c @@ -11,8 +11,19 @@ #include <err.h> #include <signal.h> #include <asm/unistd.h> +#include <linux/types.h> #include "linux/ptrace.h" +#if defined(_MIPS_SIM) && _MIPS_SIM == _MIPS_SIM_NABI32 +/*
- MIPS N32 is the only architecture where __kernel_ulong_t
- does not match the bitness of syscall arguments.
- */
+typedef unsigned long long kernel_ulong_t; +#else +typedef __kernel_ulong_t kernel_ulong_t; +#endif
What's the reason for adding these typedefs? checkpatch should have warned you about adding new typedefs.
Also this introduces kernel_ulong_t in user-space test code. Something to avoid.
There has to be a new type for this test, and the natural way to do this is to use typedef. The alternative would be to #define kernel_ulong_t which is ugly. By the way, there are quite a few typedefs in selftests, and there seems to be given no rationale why adding new types in selftests is a bad idea.
That is, the new type in this test is being added on purpose, and I'd rather keep it this way.
static int kill_tracee(pid_t pid) { @@ -42,37 +53,37 @@ sys_ptrace(int request, pid_t pid, unsigned long addr, unsigned long data) TEST(get_syscall_info) {
- static const unsigned long args[][7] = {
- const kernel_ulong_t args[][7] = { /* a sequence of architecture-agnostic syscalls */ { __NR_chdir,
(unsigned long) "",0xbad1fed1,0xbad2fed2,0xbad3fed3,0xbad4fed4,0xbad5fed5
(uintptr_t) "",You could use ifdef here.
Not just here but in other cases as well. I think this would make the code less readable. I'd prefer a single ifdef with a single explanatory comment.
On Sat, 29 Mar 2025, Dmitry V. Levin wrote:
+#if defined(_MIPS_SIM) && _MIPS_SIM == _MIPS_SIM_NABI32 +/*
- MIPS N32 is the only architecture where __kernel_ulong_t
- does not match the bitness of syscall arguments.
- */
+typedef unsigned long long kernel_ulong_t; +#else +typedef __kernel_ulong_t kernel_ulong_t; +#endif
What's the reason for adding these typedefs? checkpatch should have warned you about adding new typedefs.
Also this introduces kernel_ulong_t in user-space test code. Something to avoid.
There has to be a new type for this test, and the natural way to do this is to use typedef. The alternative would be to #define kernel_ulong_t which is ugly. By the way, there are quite a few typedefs in selftests, and there seems to be given no rationale why adding new types in selftests is a bad idea.
FWIW I agree, and I fail to see a reason why this would be a problem in a standalone test program where the typedef does not propagate anywhere.
The only potential issue I can identify would be a namespace clash, so perhaps the new type could have a name prefix specific to the test, but it doesn't appear to me a widespread practice across our selftests and then `kernel_' ought to be pretty safe against ISO C or POSIX, so perhaps let's leave the things alone?
Maciej
On 3/29/25 08:02, Maciej W. Rozycki wrote:
On Sat, 29 Mar 2025, Dmitry V. Levin wrote:
+#if defined(_MIPS_SIM) && _MIPS_SIM == _MIPS_SIM_NABI32 +/*
- MIPS N32 is the only architecture where __kernel_ulong_t
- does not match the bitness of syscall arguments.
- */
+typedef unsigned long long kernel_ulong_t; +#else +typedef __kernel_ulong_t kernel_ulong_t; +#endif
What's the reason for adding these typedefs? checkpatch should have warned you about adding new typedefs.
Also this introduces kernel_ulong_t in user-space test code. Something to avoid.
There has to be a new type for this test, and the natural way to do this is to use typedef. The alternative would be to #define kernel_ulong_t which is ugly. By the way, there are quite a few typedefs in selftests, and there seems to be given no rationale why adding new types in selftests is a bad idea.
It causes problems down the road for maintenance. I would rather not see these types of kernel typedefs added to user-space.
FWIW I agree, and I fail to see a reason why this would be a problem in a standalone test program where the typedef does not propagate anywhere.
The only potential issue I can identify would be a namespace clash, so perhaps the new type could have a name prefix specific to the test, but it doesn't appear to me a widespread practice across our selftests and then `kernel_' ought to be pretty safe against ISO C or POSIX, so perhaps let's leave the things alone?
Can't this be solved with ifdef?
thanks, -- Shuah
linux-kselftest-mirror@lists.linaro.org
-
Dmitry V. Levin -
Maciej W. Rozycki -
Shuah Khan