This makes ptrace/get_syscall_info selftest pass on mips o32 and
mips64 o32 by fixing the following two test assertions:

1. 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

2. get_syscall_info test assertion on mips64 o32:
  # 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

The first assertion is fixed for mips o32 by using struct pt_regs.pad0
instead of get_user() to obtain syscall arguments.  This approach works
due to this piece in arch/mips/kernel/scall32-o32.S:

        /*
         * 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

arch/mips/kernel/scall64-o32.S has analogous code for mips64 o32 that
allows obtaining syscall arguments from struct pt_regs.regs[4..11]
instead of get_user().

The second assertion is fixed by truncating 64-bit values to 32-bit
syscall arguments.

Signed-off-by: Dmitry V. Levin <ldv@strace.io>
---
 arch/mips/include/asm/syscall.h | 32 ++++++++------------------------
 1 file changed, 8 insertions(+), 24 deletions(-)

diff --git a/arch/mips/include/asm/syscall.h b/arch/mips/include/asm/syscall.h
index ebdf4d910af2..b3f00ede8bb3 100644
--- a/arch/mips/include/asm/syscall.h
+++ b/arch/mips/include/asm/syscall.h
@@ -57,37 +57,21 @@ 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)))
+		*arg = (unsigned int)*arg;
+#endif
 }
 
 static inline long syscall_get_error(struct task_struct *task,
-- 
ldv

On Mon, 10 Feb 2025, Dmitry V. Levin wrote:

> The first assertion is fixed for mips o32 by using struct pt_regs.pad0
> instead of get_user() to obtain syscall arguments.  This approach works
> due to this piece in arch/mips/kernel/scall32-o32.S:

 I've had a look now and I can see what's going on here.

 The thing is we're trying to access another task's context and obviously 
dereferencing $sp obtained from it is not going to work via get_user(), 
because that just peeks at the current task's context.  It often does not 
crash, because the default user stack always gets assigned the same VMA, 
but it is pure luck which we wouldn't have if the stack was switched (via 
setcontext(3) or however) or say a non-default process's thread peeked at, 
and in any case irrelevant data is obtained just as observed with the test 
case.

 We ought to be using access_remote_vm() to retrieve the other task's 
stack contents, but given that the data has been already obtained and 
saved in `struct pt_regs' it would be an overkill.

 So I think your change is actually the correct thing to do, but please 
let's not abuse a struct member called `pad', the name of which indicates 
its contents are not supposed to be of any use.  I have therefore posted a 
preparatory cleanup[1].  May you please rebase your patch on top of that 
and also update the change description so as to reflect the findings?

 Thomas, can you please apply my cleanup soon and ahead of Dmitry's change 
so as to make things easy to proceed with?  Or otherwise let me know what 
works for you best.

 Also I have a suspicion this stuff ought to be backported, but I guess it 
can be decided later on.

 Thank you for your patience.

[1] "MIPS: Export syscall stack arguments properly for remote use", 
    <https://lore.kernel.org/linux-mips/alpine.DEB.2.21.2502101732120.65342@angie.orcam.me.uk/>

  Maciej

On Tue, Feb 11, 2025 at 06:30:33PM +0000, Maciej W. Rozycki wrote:
> On Mon, 10 Feb 2025, Dmitry V. Levin wrote:
> 
> > The first assertion is fixed for mips o32 by using struct pt_regs.pad0
> > instead of get_user() to obtain syscall arguments.  This approach works
> > due to this piece in arch/mips/kernel/scall32-o32.S:
> 
>  I've had a look now and I can see what's going on here.
> 
>  The thing is we're trying to access another task's context and obviously 
> dereferencing $sp obtained from it is not going to work via get_user(), 
> because that just peeks at the current task's context.  It often does not 
> crash, because the default user stack always gets assigned the same VMA, 
> but it is pure luck which we wouldn't have if the stack was switched (via 
> setcontext(3) or however) or say a non-default process's thread peeked at, 
> and in any case irrelevant data is obtained just as observed with the test 
> case.
> 
>  We ought to be using access_remote_vm() to retrieve the other task's 
> stack contents, but given that the data has been already obtained and 
> saved in `struct pt_regs' it would be an overkill.
> 
>  So I think your change is actually the correct thing to do, but please 
> let's not abuse a struct member called `pad', the name of which indicates 
> its contents are not supposed to be of any use.  I have therefore posted a 
> preparatory cleanup[1].  May you please rebase your patch on top of that 
> and also update the change description so as to reflect the findings?
> 
>  Thomas, can you please apply my cleanup soon and ahead of Dmitry's change 
> so as to make things easy to proceed with?  Or otherwise let me know what 
> works for you best.
> 
>  Also I have a suspicion this stuff ought to be backported, but I guess it 
> can be decided later on.
> 
>  Thank you for your patience.
> 
> [1] "MIPS: Export syscall stack arguments properly for remote use", 
>     <https://lore.kernel.org/linux-mips/alpine.DEB.2.21.2502101732120.65342@angie.orcam.me.uk/>

Thanks for the analysis, I'm going to rebase my fix and send it
as a follow-up to your cleanup patch.


-- 
ldv