Re: [RFC PATCH 2/2] riscv: selftests: Add a ptrace test to check a0 of restarted syscall

[Charlie Jenkins]

On Fri, Jun 21, 2024 at 02:29:07PM +0800, Quan Zhou wrote:
> 
> 
> On 2024/6/20 10:55, Charlie Jenkins wrote:
> > On Wed, Jun 19, 2024 at 10:01:47AM +0800, zhouquan@xxxxxxxxxxx wrote:
> > > From: Quan Zhou <zhouquan@xxxxxxxxxxx>
> > > 
> > > This test creates two processes: a tracer and a tracee. The tracer actively
> > > sends a SIGUSR1 signal in user mode to interrupt the read syscall being
> > > executed by the tracee. We will reset a0/orig_a0 and then observe the value
> > > of a0 held by the restarted read syscall.
> > 
> > I don't quite follow what the goal of this test is. With orig_a0 being
> > added to the previous patch for ptrace, a more constrained test could
> > ensure that this value is respected.
> > 
> 
> Sry, I may not have described the patch clearly enough. This patch provides
> a channel for modifying a0 in user-space ptrace via orig_a0. Here, I will
> try to outline the whole situation:
> 
> 1、When the tracer calls ptrace to modify regs->a0, can the tracee's a0 be
> correctly modified?
> 
> Through testing, if the user only modifies regs->a0, it doesn't work. Why?
> 
> The execution flow of the tracee in the test program is as follows.Prior to
> this explanation:
> 
> - PTRACE_SYSCALL can make the tracee block before and after executing
>   a syscall.
> - The tracer sends SIGUSR1 to interrupt read, and the kernel will
>   restart it.
> - Please note the point marked with (*), which I believe is the cause
>   of the issue.
> 
> user     kernel
>     |
>     |
>     |
>      read
>         | +-> regs->orig_a0 = regs->a0; //(*1)
>         |                                       <=tracer:PTRACE_SYSCALL
>         | +-> syscall_enter_from_user_mode
>               +-> ptrace_report_syscall_entry
>                   +-> ptrace_stop
>         | //stopped
>         |                                       <= tracer:SIGUSR1
>         |
>         | //resume                              <= tracer:PTRACE_SYSCALL
>         | syscall_handler...
>         |
>         | +-> syscall_exit_to_user_mode
>               +-> syscall_exit_to_user_mode_prepare
>                   +-> ptrace_report_syscall_exit
>                       +-> ptrace_stop
>     | //stopped
>     |
>     | /* Change a0/orig_a0 here and observe the restarted syscall */
>     | regs->{a0/orig_a0} = fd_zero; //(*2)
>     | ptrace(PTRACE_SETREGSET, ...);
>         |                                      <= tracer:PTRACE_SYSCALL
>         | //restarting..., skip SIGUSR1
>         |
>         | +-> exit_to_user_mode_loop
>               +-> arch_do_signal_or_restart
>                   +-> /* Settings for syscall restart */
>                       regs->a0 = regs->orig_a0; //(*3)
>     | //stopped
>     | //and block before the syscall again, get current regs->a0
>     | *result = regs->a0;
>     |
>     | /* Now, Check regs->a0 of restarted syscall */
>     | EXPECT_NE(0x5, result); //for PTRACE_SETREGSSET a0, failed
>     | EXPECT_EQ(0x5, result); //for PTRACE_SETREGSSET orig_a0, succeed
> 
> If I'm wrong, please let me know. 🙂
> 
> 2、Actually, I discovered the issue while using the execve function.
> When I tried to modify the first parameter of execve in the tracer,
> I found it didn't work.
> 
> As for why not use execve for testing, there are two reasons:
> 
> 1) The root cause of this issue is that when a syscall is interrupted
> and then resumed, it restarts with orig_a0 instead of a0, so modifying
> a0 doesn't work. I want to focus the test on the "restarted syscall".
> 
> 2) Compared to the current test scenario, execve is terminated by ptrace
> earlier, so I chose a later point. In fact, setting regs->a0 in the path
> between (*1) and (*3) is ineffective because it will eventually be
> overwritten by orig_a0, correct?

Thank you for the thorough explanation! I feel like a test case like the
following achieves the same goal but without needing the pipes and the
file. What do you think?