[PATCH v2] selftests/ptrace/get_syscall_info: fix for MIPS n32

[PATCH v2] selftests/ptrace/get_syscall_info: fix for MIPS n32

[Dmitry V. Levin]

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) {
-- 
ldv

Re: [PATCH v2] selftests/ptrace/get_syscall_info: fix for MIPS n32

[Dmitry V. Levin]

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
>  		},
>  		{
>  			__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) {
> -- 
> ldv

-- 
ldv

Re: [PATCH v2] selftests/ptrace/get_syscall_info: fix for MIPS n32

[Shuah Khan]

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
>   		},
>   		{
>   			__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) {

thanks,
-- Shuah

Re: [PATCH v2] selftests/ptrace/get_syscall_info: fix for MIPS n32

[Dmitry V. Levin]

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.


-- 
ldv

Re: [PATCH v2] selftests/ptrace/get_syscall_info: fix for MIPS n32

[Maciej W. Rozycki]

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

Re: [PATCH v2] selftests/ptrace/get_syscall_info: fix for MIPS n32

[Shuah Khan]

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

Re: [PATCH v2] selftests/ptrace/get_syscall_info: fix for MIPS n32

[Dmitry V. Levin]

On Sat, Mar 29, 2025 at 02:02:28PM +0000, 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.
> 
>  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?

Another similar test I authored (selftests/ptrace/set_syscall_info) has
been merged, so there are two similar tests in the tree now, but only
one of them is permitted to use this approach, creating inconsistency.

Taking all of the above into consideration, please approve this fix.


-- 
ldv

Re: [PATCH v2] selftests/ptrace/get_syscall_info: fix for MIPS n32

[Shuah Khan]

On 6/2/25 05:59, Dmitry V. Levin wrote:
> On Sat, Mar 29, 2025 at 02:02:28PM +0000, 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.
>>
>>   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?
> 
> Another similar test I authored (selftests/ptrace/set_syscall_info) has
> been merged, so there are two similar tests in the tree now, but only
> one of them is permitted to use this approach, creating inconsistency.
> 
> Taking all of the above into consideration, please approve this fix.
> 
> 

Acked-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah