Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request

[Charlie Jenkins <charlie@rivosinc.com>]

On Thu, Jan 16, 2025 at 01:07:59PM -0800, Charlie Jenkins wrote:
> On Thu, Jan 16, 2025 at 10:33:28AM +0200, Dmitry V. Levin wrote:
> > On Wed, Jan 15, 2025 at 05:55:31PM -0800, Charlie Jenkins wrote:
> > > On Mon, Jan 13, 2025 at 07:12:08PM +0200, Dmitry V. Levin wrote:
> > [...]
> > > > +	/* Changing the type of the system call stop is not supported. */
> > > > +	if (ptrace_get_syscall_info_op(child) != info.op)
> > > 
> > > Since this isn't supported anyway, would it make sense to set the
> > > info.op to ptrace_get_syscall_info_op(child) like is done for
> > > get_syscall_info? The usecase I see for this is simplifying when the
> > > user doesn't call PTRACE_GET_SYSCALL_INFO before calling
> > > PTRACE_SET_SYSCALL_INFO.
> > 
> > struct ptrace_syscall_info.op is a field that specifies how to interpret
> > the union fields of the structure, so if "op" is ignored, then the
> > kernel would infer the meaning of the structure specified by the userspace
> > tracer from the kernel state of the tracee.  This looks a bit too
> > error-prone to allow.  For example, nothing good is expected to happen
> > if syscall entry information is applied in a syscall exit stop.
> 
> Yes that's a good point. 
> 
> > 
> > The tracer is not obliged to call PTRACE_GET_SYSCALL_INFO to set
> > struct ptrace_syscall_info.op.  If the tracer keeps track of ptrace stops
> > by other means, it can assign the right value by itself.
> >
> > And, btw, the comment should say "is not currently supported",
> > I'll update it in the next iteration.
> > 
> > An idea mentioned in prior discussions was that it would make sense to
> > specify syscall return value along with skipping the syscall in seccomp stop,
> > and this would require a different value for "op" field, but
> > I decided not to introduce this extra complexity yet.
> 
> Makes sense, thank you!
> 
> - Charlie

I am no longer convinced that we need Celeste's patch that solves this
problem on riscv [1]. That patch is necessary without this change, but
PTRACE_SET_SYSCALL_INFO seems like a cleaner solution.

Reviewed-by: Charlie Jenkins <charlie@rivosinc.com>
Tested-by: Charlie Jenkins <charlie@rivosinc.com>

- Charlie

[1] https://lore.kernel.org/lkml/20250115-13cc73c36c7bb3b9f046f614@orel/T/

> 
> > 
> > 
> > -- 
> > ldv