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

["Dmitry V. Levin" <ldv@strace.io>]

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