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 <l...@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
