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

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:

quoted

On Wed, Jan 15, 2025 at 05:55:31PM -0800, Charlie Jenkins wrote:

quoted

On Mon, Jan 13, 2025 at 07:12:08PM +0200, Dmitry V. Levin wrote:

[...]

quoted

quoted

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

quoted

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 <redacted>
Tested-by: Charlie Jenkins <redacted>

- Charlie

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

quoted

-- 
ldv