| Message ID | 20250113171114.GB589@strace.io (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ptrace: introduce PTRACE_SET_SYSCALL_INFO API | expand |
On Mon, 13 Jan 2025, Dmitry V. Levin wrote: > Fix the following get_syscall_info test assertion on mips O32: > # get_syscall_info.c:218:get_syscall_info:Expected exp_args[5] (3134521044) == info.entry.args[4] (4911432) > # get_syscall_info.c:219:get_syscall_info:wait #1: entry stop mismatch > > Fix the following get_syscall_info test assertion on mips64 O32 and mips64 N32: > # get_syscall_info.c:209:get_syscall_info:Expected exp_args[2] (3134324433) == info.entry.args[1] (18446744072548908753) > # get_syscall_info.c:210:get_syscall_info:wait #1: entry stop mismatch How did you produce these results? > This makes ptrace/get_syscall_info selftest pass on mips O32, > mips64 O32, and mips64 N32. > > Signed-off-by: Dmitry V. Levin <ldv@strace.io> > --- > > Note that I'm not a MIPS expert, so I cannot tell why the get_user() > approach doesn't work for O32. Also, during experiments I discovered that > regs->pad0 approach works for O32, but why it works remains a mystery. The patch is definitely broken, the calling convention is the same between n32 and n64: 64-bit arguments in $4 through $11 registers as required, and your change makes n32 truncate arguments to 32 bits. The regs->pad0 approach works due to this piece: /* * Ok, copy the args from the luser stack to the kernel stack. */ .set push .set noreorder .set nomacro load_a4: user_lw(t5, 16(t0)) # argument #5 from usp load_a5: user_lw(t6, 20(t0)) # argument #6 from usp load_a6: user_lw(t7, 24(t0)) # argument #7 from usp load_a7: user_lw(t8, 28(t0)) # argument #8 from usp loads_done: sw t5, 16(sp) # argument #5 to ksp sw t6, 20(sp) # argument #6 to ksp sw t7, 24(sp) # argument #7 to ksp sw t8, 28(sp) # argument #8 to ksp .set pop .section __ex_table,"a" PTR_WD load_a4, bad_stack_a4 PTR_WD load_a5, bad_stack_a5 PTR_WD load_a6, bad_stack_a6 PTR_WD load_a7, bad_stack_a7 .previous in arch/mips/kernel/scall32-o32.S (and arch/mips/kernel/scall64-o32.S has analogous code to adapt to the native n64 calling convention instead), but this doesn't seem to me to be the correct approach here. At first glance `mips_get_syscall_arg' does appear fine as it is, so I'd like to know how you obtained your results. Maciej
On Tue, Jan 14, 2025 at 03:29:11AM +0000, Maciej W. Rozycki wrote: > On Mon, 13 Jan 2025, Dmitry V. Levin wrote: > > > Fix the following get_syscall_info test assertion on mips O32: > > # get_syscall_info.c:218:get_syscall_info:Expected exp_args[5] (3134521044) == info.entry.args[4] (4911432) > > # get_syscall_info.c:219:get_syscall_info:wait #1: entry stop mismatch > > > > Fix the following get_syscall_info test assertion on mips64 O32 and mips64 N32: > > # get_syscall_info.c:209:get_syscall_info:Expected exp_args[2] (3134324433) == info.entry.args[1] (18446744072548908753) > > # get_syscall_info.c:210:get_syscall_info:wait #1: entry stop mismatch > > How did you produce these results? $ PATH="$HOME/x-tools/mips64-unknown-linux-gnu/bin:$PATH" make -j`nproc` ARCH=mips CROSS_COMPILE=mips64-unknown-linux-gnu- -C tools/testing/selftests TARGETS=ptrace USERLDFLAGS='-static' USERCFLAGS='-mabi=32' $ echo init |(cd tools/testing/selftests/ptrace && ln -snf get_syscall_info init && cpio --dereference -o -H newc -R 0:0) |gzip >get_syscall_info.mips-o32.img $ qemu-system-mips -nographic -kernel vmlinuz -initrd get_syscall_info.mips-o32.img -append 'console=ttyS0' Likewise for mips64, but the patch for kselftest_harness.h from [1] is needed to see correct mismatch values in the test diagnostics. [1] https://lore.kernel.org/all/20250108170757.GA6723@strace.io/ > > This makes ptrace/get_syscall_info selftest pass on mips O32, > > mips64 O32, and mips64 N32. > > > > Signed-off-by: Dmitry V. Levin <ldv@strace.io> > > --- > > > > Note that I'm not a MIPS expert, so I cannot tell why the get_user() > > approach doesn't work for O32. Also, during experiments I discovered that > > regs->pad0 approach works for O32, but why it works remains a mystery. > > The patch is definitely broken, the calling convention is the same > between n32 and n64: 64-bit arguments in $4 through $11 registers as > required, and your change makes n32 truncate arguments to 32 bits. There must be something very specific to n32 then: apparently, __kernel_ulong_t is a 32-bit type on n32, so the syscall arguments are 32-bit values, at some point (in glibc?) they get sign-extended from 32 to 64 bits, and syscall_get_arguments returns them as 64-bit values different from the original syscall arguments, breaking the test. If this is the expected behaviour, then I'd have to add an exception for mips n32 both to the kernel test and to strace that uses this interface. > The regs->pad0 approach works due to this piece: > > /* > * Ok, copy the args from the luser stack to the kernel stack. > */ > > .set push > .set noreorder > .set nomacro > > load_a4: user_lw(t5, 16(t0)) # argument #5 from usp > load_a5: user_lw(t6, 20(t0)) # argument #6 from usp > load_a6: user_lw(t7, 24(t0)) # argument #7 from usp > load_a7: user_lw(t8, 28(t0)) # argument #8 from usp > loads_done: > > sw t5, 16(sp) # argument #5 to ksp > sw t6, 20(sp) # argument #6 to ksp > sw t7, 24(sp) # argument #7 to ksp > sw t8, 28(sp) # argument #8 to ksp > .set pop > > .section __ex_table,"a" > PTR_WD load_a4, bad_stack_a4 > PTR_WD load_a5, bad_stack_a5 > PTR_WD load_a6, bad_stack_a6 > PTR_WD load_a7, bad_stack_a7 > .previous > > in arch/mips/kernel/scall32-o32.S (and arch/mips/kernel/scall64-o32.S has > analogous code to adapt to the native n64 calling convention instead), but > this doesn't seem to me to be the correct approach here. At first glance > `mips_get_syscall_arg' does appear fine as it is, so I'd like to know how > you obtained your results. > > Maciej
On Tue, 14 Jan 2025, Dmitry V. Levin wrote: > > How did you produce these results? > > $ PATH="$HOME/x-tools/mips64-unknown-linux-gnu/bin:$PATH" make -j`nproc` ARCH=mips CROSS_COMPILE=mips64-unknown-linux-gnu- -C tools/testing/selftests TARGETS=ptrace USERLDFLAGS='-static' USERCFLAGS='-mabi=32' > $ echo init |(cd tools/testing/selftests/ptrace && ln -snf get_syscall_info init && cpio --dereference -o -H newc -R 0:0) |gzip >get_syscall_info.mips-o32.img > $ qemu-system-mips -nographic -kernel vmlinuz -initrd get_syscall_info.mips-o32.img -append 'console=ttyS0' > > Likewise for mips64, but the patch for kselftest_harness.h from [1] > is needed to see correct mismatch values in the test diagnostics. > > [1] https://lore.kernel.org/all/20250108170757.GA6723@strace.io/ Thanks, I'll try to see what's going on with `get_user'. > > The patch is definitely broken, the calling convention is the same > > between n32 and n64: 64-bit arguments in $4 through $11 registers as > > required, and your change makes n32 truncate arguments to 32 bits. > > There must be something very specific to n32 then: apparently, > __kernel_ulong_t is a 32-bit type on n32, so the syscall arguments are > 32-bit values, at some point (in glibc?) they get sign-extended from 32 to > 64 bits, and syscall_get_arguments returns them as 64-bit values different > from the original syscall arguments, breaking the test. This matters at least for `lseek', which has an `off64_t' aka `long long' argument on n32 (there's no `_llseek' on n32). Since arguments are passed via 64-bit registers and a `long long' datum is held in just one this is transparent between n32 and n64 (of course on n64 this corresponds to the plain `long' data type, so the kernel, which is always n64 for 64-bit configurations, sees the incoming argument as `long', and the same stands for the outgoing return value). Surely non-LFS lseek(2) will produce the syscall's `long long' argument truncated (cf. sysdeps/unix/sysv/linux/mips/mips64/n32/lseek.c in glibc), but both LFS lseek(2) and lseek64(2) will pass the native value on n32. > If this is the expected behaviour, then I'd have to add an exception for > mips n32 both to the kernel test and to strace that uses this interface. Is MIPS n32 the only psABI across all our architectures supported that can have `long long' syscall arguments? I guess it might actually be the case, in which case I won't be surprised it needs specific handling. Maciej
On Tue, Jan 14, 2025 at 04:03:28PM +0000, Maciej W. Rozycki wrote: > On Tue, 14 Jan 2025, Dmitry V. Levin wrote: > > > > How did you produce these results? > > > > $ PATH="$HOME/x-tools/mips64-unknown-linux-gnu/bin:$PATH" make -j`nproc` ARCH=mips CROSS_COMPILE=mips64-unknown-linux-gnu- -C tools/testing/selftests TARGETS=ptrace USERLDFLAGS='-static' USERCFLAGS='-mabi=32' > > $ echo init |(cd tools/testing/selftests/ptrace && ln -snf get_syscall_info init && cpio --dereference -o -H newc -R 0:0) |gzip >get_syscall_info.mips-o32.img > > $ qemu-system-mips -nographic -kernel vmlinuz -initrd get_syscall_info.mips-o32.img -append 'console=ttyS0' > > > > Likewise for mips64, but the patch for kselftest_harness.h from [1] > > is needed to see correct mismatch values in the test diagnostics. > > > > [1] https://lore.kernel.org/all/20250108170757.GA6723@strace.io/ > > Thanks, I'll try to see what's going on with `get_user'. Thanks. > > > The patch is definitely broken, the calling convention is the same > > > between n32 and n64: 64-bit arguments in $4 through $11 registers as > > > required, and your change makes n32 truncate arguments to 32 bits. > > > > There must be something very specific to n32 then: apparently, > > __kernel_ulong_t is a 32-bit type on n32, so the syscall arguments are > > 32-bit values, at some point (in glibc?) they get sign-extended from 32 to > > 64 bits, and syscall_get_arguments returns them as 64-bit values different > > from the original syscall arguments, breaking the test. > > This matters at least for `lseek', which has an `off64_t' aka `long long' > argument on n32 (there's no `_llseek' on n32). Since arguments are passed > via 64-bit registers and a `long long' datum is held in just one this is > transparent between n32 and n64 (of course on n64 this corresponds to the > plain `long' data type, so the kernel, which is always n64 for 64-bit > configurations, sees the incoming argument as `long', and the same stands > for the outgoing return value). > > Surely non-LFS lseek(2) will produce the syscall's `long long' argument > truncated (cf. sysdeps/unix/sysv/linux/mips/mips64/n32/lseek.c in glibc), > but both LFS lseek(2) and lseek64(2) will pass the native value on n32. > > > If this is the expected behaviour, then I'd have to add an exception for > > mips n32 both to the kernel test and to strace that uses this interface. > > Is MIPS n32 the only psABI across all our architectures supported that > can have `long long' syscall arguments? I guess it might actually be the > case, in which case I won't be surprised it needs specific handling. This is very similar to x32, with the only essential difference: on x32 the 64-bitness of syscall arguments is exposed to userspace via __kernel_ulong_t: arch/x86/include/uapi/asm/posix_types_x32.h:typedef unsigned long long __kernel_ulong_t; In fact, there is a workaround in strace for this case, and now I realized that it ceased to work on mips n32 long time ago: # if defined HAVE___KERNEL_LONG_T && defined HAVE___KERNEL_ULONG_T # include <asm/posix_types.h> typedef __kernel_long_t kernel_long_t; typedef __kernel_ulong_t kernel_ulong_t; # elif (defined __x86_64__ && defined __ILP32__) || defined LINUX_MIPSN32 typedef long long kernel_long_t; typedef unsigned long long kernel_ulong_t; # else typedef long kernel_long_t; typedef unsigned long kernel_ulong_t; # endif
diff --git a/arch/mips/include/asm/syscall.h b/arch/mips/include/asm/syscall.h index ebdf4d910af2..2f85f2d8f754 100644 --- a/arch/mips/include/asm/syscall.h +++ b/arch/mips/include/asm/syscall.h @@ -57,37 +57,23 @@ static inline void mips_syscall_update_nr(struct task_struct *task, static inline void mips_get_syscall_arg(unsigned long *arg, struct task_struct *task, struct pt_regs *regs, unsigned int n) { - unsigned long usp __maybe_unused = regs->regs[29]; - +#ifdef CONFIG_32BIT switch (n) { case 0: case 1: case 2: case 3: *arg = regs->regs[4 + n]; - - return; - -#ifdef CONFIG_32BIT - case 4: case 5: case 6: case 7: - get_user(*arg, (int *)usp + n); return; -#endif - -#ifdef CONFIG_64BIT case 4: case 5: case 6: case 7: -#ifdef CONFIG_MIPS32_O32 - if (test_tsk_thread_flag(task, TIF_32BIT_REGS)) - get_user(*arg, (int *)usp + n); - else -#endif - *arg = regs->regs[4 + n]; - + *arg = regs->pad0[n]; return; -#endif - - default: - BUG(); } - - unreachable(); +#else + *arg = regs->regs[4 + n]; + if ((IS_ENABLED(CONFIG_MIPS32_O32) && + test_tsk_thread_flag(task, TIF_32BIT_REGS)) || + (IS_ENABLED(CONFIG_MIPS32_N32) && + test_tsk_thread_flag(task, TIF_32BIT_ADDR))) + *arg = (unsigned int)*arg; +#endif } static inline long syscall_get_error(struct task_struct *task,
Fix the following get_syscall_info test assertion on mips O32: # get_syscall_info.c:218:get_syscall_info:Expected exp_args[5] (3134521044) == info.entry.args[4] (4911432) # get_syscall_info.c:219:get_syscall_info:wait #1: entry stop mismatch Fix the following get_syscall_info test assertion on mips64 O32 and mips64 N32: # get_syscall_info.c:209:get_syscall_info:Expected exp_args[2] (3134324433) == info.entry.args[1] (18446744072548908753) # get_syscall_info.c:210:get_syscall_info:wait #1: entry stop mismatch This makes ptrace/get_syscall_info selftest pass on mips O32, mips64 O32, and mips64 N32. Signed-off-by: Dmitry V. Levin <ldv@strace.io> --- Note that I'm not a MIPS expert, so I cannot tell why the get_user() approach doesn't work for O32. Also, during experiments I discovered that regs->pad0 approach works for O32, but why it works remains a mystery. arch/mips/include/asm/syscall.h | 34 ++++++++++----------------------- 1 file changed, 10 insertions(+), 24 deletions(-)