diff mbox series

[v5,2/2] riscv: selftests: Add a ptrace test to verify a0 and orig_a0 access

Message ID 20250115-riscv-new-regset-v5-2-d0e6ec031a23@coelacanthus.name (mailing list archive)
State New
Headers show
Series riscv/ptrace: add new regset to access original a0 register | expand

Commit Message

Celeste Liu Jan. 14, 2025, 8:24 p.m. UTC
This test checks that orig_a0 and a0 can be modified and accessed
independently.

Co-developed-by: Quan Zhou <zhouquan@iscas.ac.cn>
Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
Co-developed-by: Charlie Jenkins <charlie@rivosinc.com>
Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
Reviewed-by: Björn Töpel <bjorn@rivosinc.com>
Signed-off-by: Celeste Liu <uwu@coelacanthus.name>
---
 tools/testing/selftests/riscv/abi/.gitignore |   1 +
 tools/testing/selftests/riscv/abi/Makefile   |   6 +-
 tools/testing/selftests/riscv/abi/ptrace.c   | 201 +++++++++++++++++++++++++++
 3 files changed, 207 insertions(+), 1 deletion(-)

Comments

Andrew Jones Jan. 15, 2025, 9:14 a.m. UTC | #1
On Wed, Jan 15, 2025 at 04:24:59AM +0800, Celeste Liu wrote:
> This test checks that orig_a0 and a0 can be modified and accessed
> independently.
> 
> Co-developed-by: Quan Zhou <zhouquan@iscas.ac.cn>
> Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
> Co-developed-by: Charlie Jenkins <charlie@rivosinc.com>
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> Reviewed-by: Björn Töpel <bjorn@rivosinc.com>
> Signed-off-by: Celeste Liu <uwu@coelacanthus.name>
> ---
>  tools/testing/selftests/riscv/abi/.gitignore |   1 +
>  tools/testing/selftests/riscv/abi/Makefile   |   6 +-
>  tools/testing/selftests/riscv/abi/ptrace.c   | 201 +++++++++++++++++++++++++++
>  3 files changed, 207 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/riscv/abi/.gitignore b/tools/testing/selftests/riscv/abi/.gitignore
> index b38358f91c4d2240ae64892871d9ca98bda1ae58..378c605919a3b3d58eec2701eb7af430cfe315d6 100644
> --- a/tools/testing/selftests/riscv/abi/.gitignore
> +++ b/tools/testing/selftests/riscv/abi/.gitignore
> @@ -1 +1,2 @@
>  pointer_masking
> +ptrace
> diff --git a/tools/testing/selftests/riscv/abi/Makefile b/tools/testing/selftests/riscv/abi/Makefile
> index ed82ff9c664e7eb3f760cbab81fb957ff72579c5..359a082c88a401883fb3776b35e4dacf69beaaaa 100644
> --- a/tools/testing/selftests/riscv/abi/Makefile
> +++ b/tools/testing/selftests/riscv/abi/Makefile
> @@ -1,10 +1,14 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
>  CFLAGS += -I$(top_srcdir)/tools/include
> +CFLAGS += $(KHDR_INCLUDES)
>  
> -TEST_GEN_PROGS := pointer_masking
> +TEST_GEN_PROGS := pointer_masking ptrace
>  
>  include ../../lib.mk
>  
>  $(OUTPUT)/pointer_masking: pointer_masking.c
>  	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
> +
> +$(OUTPUT)/ptrace: ptrace.c
> +	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
> diff --git a/tools/testing/selftests/riscv/abi/ptrace.c b/tools/testing/selftests/riscv/abi/ptrace.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..f1a0458adccdd040bfaa350e2e8d98b1ef34c0ad
> --- /dev/null
> +++ b/tools/testing/selftests/riscv/abi/ptrace.c
> @@ -0,0 +1,201 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <signal.h>
> +#include <errno.h>
> +#include <sys/types.h>
> +#include <sys/ptrace.h>
> +#include <sys/stat.h>
> +#include <sys/user.h>
> +#include <sys/wait.h>
> +#include <sys/uio.h>
> +#include <linux/elf.h>
> +#include <linux/unistd.h>
> +#include <linux/ptrace.h>
> +#include <asm/ptrace.h>
> +
> +#include "../../kselftest_harness.h"
> +
> +#ifndef sizeof_field
> +#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
> +#endif
> +#ifndef offsetofend
> +#define offsetofend(TYPE, MEMBER) \
> +	(offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
> +#endif

I think this is the sixth test to define these. We should copy
include/linux/stddef.h into tools/include. We already have
tools/include/uapi/linux/stddef.h with __struct_group and
__DECLARE_FLEX_ARRAY, so I think it should just work.

> +
> +
> +#define ORIG_A0_MODIFY      0x01
> +#define A0_MODIFY           0x02
> +#define A0_OLD              0xbadbeefbeeff
> +#define A0_NEW              0xffeebfeebdab
> +
> +
> +struct a0_regs {
> +	__s64 orig_a0;
> +	__u64 a0;
> +};
> +
> +#define perr_and_exit(fmt, ...)						\
> +	({								\
> +		char buf[256];						\
> +		snprintf(buf, sizeof(buf), "%s:%d:" fmt ": %m\n",	\
> +			__func__, __LINE__, ##__VA_ARGS__);		\
> +		ksft_exit_fail_perror(buf);				\
> +	})
> +
> +static void ptrace_test(int opt, struct a0_regs result[])
> +{
> +	int status;
> +	long rc;
> +	pid_t pid;
> +	struct user_regs_struct regs;
> +	struct iovec iov = {
> +		.iov_base = &regs,
> +		.iov_len = sizeof(regs),
> +	};
> +
> +	unsigned long orig_a0;
> +	struct iovec a0_iov = {
> +		.iov_base = &orig_a0,
> +		.iov_len = sizeof(orig_a0),
> +	};
> +	struct ptrace_syscall_info syscall_info = {
> +		.op = 0xff,
> +	};
> +	const unsigned int expected_sci_entry_size =
> +		offsetofend(struct ptrace_syscall_info, entry.args);
> +	const unsigned int expected_sci_exit_size =
> +		offsetofend(struct ptrace_syscall_info, exit.is_error);
> +
> +	pid = fork();
> +	if (pid == 0) {
> +		/* Mark oneself being traced */
> +		long val = ptrace(PTRACE_TRACEME, 0, 0, 0);
> +
> +		if (val < 0)
> +			perr_and_exit("failed to request for tracer to trace me: %ld", val);
> +
> +		kill(getpid(), SIGSTOP);
> +
> +		/* Perform chdir syscall that will be intercepted */
> +		syscall(__NR_chdir, A0_OLD);
> +
> +		exit(0);
> +	}
> +
> +	if (pid < 0)
> +		ksft_exit_fail_perror("failed to fork");
> +
> +	for (int i = 0; i < 3; i++) {
> +		if (waitpid(pid, &status, 0) != pid)
> +			perr_and_exit("failed to wait for the tracee %d", pid);
> +		if (WIFSTOPPED(status)) {
> +			switch (WSTOPSIG(status)) {
> +			case SIGSTOP:
> +				if (ptrace(PTRACE_SETOPTIONS, pid, 0, PTRACE_O_TRACESYSGOOD) < 0)
> +					perr_and_exit("failed to set PTRACE_O_TRACESYSGOOD");
> +				break;
> +			case SIGTRAP|0x80:
> +				/* Modify twice so GET_SYSCALL_INFO get modified a0 and orig_a0 */
> +				if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
> +					perr_and_exit("failed to get tracee registers");
> +				if (ptrace(PTRACE_GETREGSET, pid, NT_RISCV_ORIG_A0, &a0_iov))
> +					perr_and_exit("failed to get tracee registers");
> +
> +				/* Modify a0/orig_a0 for the syscall */
> +				switch (opt) {
> +				case A0_MODIFY:
> +					regs.a0 = A0_NEW;
> +					break;
> +				case ORIG_A0_MODIFY:
> +					orig_a0 = A0_NEW;
> +					break;
> +				}
> +
> +				if (ptrace(PTRACE_SETREGSET, pid, NT_PRSTATUS, &iov))
> +					perr_and_exit("failed to set tracee registers");
> +				if (ptrace(PTRACE_SETREGSET, pid, NT_RISCV_ORIG_A0, &a0_iov))
> +					perr_and_exit("failed to set tracee registers");
> +				switch (i) {
> +				case 1:
> +					/* Stop at the beginning of syscall */
> +					rc = ptrace(PTRACE_GET_SYSCALL_INFO, pid,
> +						sizeof(syscall_info), &syscall_info);
> +					if (rc < 0)
> +						perr_and_exit("failed to get syscall info of entry");
> +					if (rc < expected_sci_entry_size
> +						|| syscall_info.op != PTRACE_SYSCALL_INFO_ENTRY)
> +						perr_and_exit("stop position of entry mismatched");
> +					result[0].orig_a0 = syscall_info.entry.args[0];
> +					break;
> +
> +				case 2:
> +					/* Stop at the end of syscall */
> +					rc = ptrace(PTRACE_GET_SYSCALL_INFO, pid,
> +						sizeof(syscall_info), &syscall_info);
> +					if (rc < 0)
> +						perr_and_exit("failed to get syscall info of entry");
> +					if (rc < expected_sci_exit_size
> +						|| syscall_info.op != PTRACE_SYSCALL_INFO_EXIT)
> +						perr_and_exit("stop position of exit mismatched");
> +					result[0].a0 = syscall_info.exit.rval;
> +
> +					if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
> +						perr_and_exit("failed to get tracee registers");
> +					result[1].a0 = regs.a0;
> +					if (ptrace(PTRACE_GETREGSET, pid, NT_RISCV_ORIG_A0,
> +						   &a0_iov))
> +						perr_and_exit("failed to get tracee registers");
> +					result[1].orig_a0 = orig_a0;
> +				}
> +			}
> +			if (ptrace(PTRACE_SYSCALL, pid, 0, 0) < 0)
> +				perr_and_exit("failed to resume tracee");
> +		}
> +	}
> +
> +	/* Resume the tracee */
> +	ptrace(PTRACE_CONT, pid, 0, 0);
> +	if (waitpid(pid, &status, 0) != pid)
> +		perr_and_exit("failed to wait for the tracee");
> +
> +}
> +
> +TEST(ptrace_access_a0)
> +{
> +	struct a0_regs result[2];
> +
> +	ptrace_test(A0_MODIFY, result);
> +
> +	/* Verify PTRACE_SETREGSET */
> +	/* The modification of a0 cannot affect the first argument of the syscall */
> +	EXPECT_EQ(A0_OLD, result[0].orig_a0);
> +	EXPECT_EQ(A0_NEW, result[0].a0);
> +
> +	/* Verify PTRACE_GETREGSET */
> +	EXPECT_EQ(result[1].orig_a0, result[0].orig_a0);
> +	EXPECT_EQ(result[1].a0, result[0].a0);
> +}
> +
> +TEST(ptrace_access_orig_a0)
> +{
> +	struct a0_regs result[2];
> +
> +	ptrace_test(ORIG_A0_MODIFY, result);
> +
> +	/* Verify PTRACE_SETREGSET */
> +	/* Only modify orig_a0 to change the first argument of the syscall */
> +	EXPECT_EQ(A0_NEW, result[0].orig_a0);
> +	/* a0 will keep default value, orig_a0 or -ENOSYS, depends on internal. */
> +	EXPECT_NE(A0_NEW, result[0].a0);

I don't understand this test. Why don't we know what to expect? Also, the
comment says orig_a0 is an option for the value, but then we don't expect
it to be A0_NEW, even though we expect orig_a0 to be A0_NEW?

> +
> +	/* Verify PTRACE_GETREGSET */
> +	EXPECT_EQ(result[1].orig_a0, result[0].orig_a0);
> +	EXPECT_EQ(result[1].a0, result[0].a0);
> +}
> +
> +TEST_HARNESS_MAIN
> 
> -- 
> 2.48.0
>

Other than the two comments/questions, this test looks great.

Thanks,
drew
Celeste Liu Jan. 15, 2025, 9:41 a.m. UTC | #2
On 2025-01-15 17:14, Andrew Jones wrote:
> On Wed, Jan 15, 2025 at 04:24:59AM +0800, Celeste Liu wrote:
>> This test checks that orig_a0 and a0 can be modified and accessed
>> independently.
>>
>> Co-developed-by: Quan Zhou <zhouquan@iscas.ac.cn>
>> Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
>> Co-developed-by: Charlie Jenkins <charlie@rivosinc.com>
>> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
>> Reviewed-by: Björn Töpel <bjorn@rivosinc.com>
>> Signed-off-by: Celeste Liu <uwu@coelacanthus.name>
>> ---
>>  tools/testing/selftests/riscv/abi/.gitignore |   1 +
>>  tools/testing/selftests/riscv/abi/Makefile   |   6 +-
>>  tools/testing/selftests/riscv/abi/ptrace.c   | 201 +++++++++++++++++++++++++++
>>  3 files changed, 207 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/riscv/abi/.gitignore b/tools/testing/selftests/riscv/abi/.gitignore
>> index b38358f91c4d2240ae64892871d9ca98bda1ae58..378c605919a3b3d58eec2701eb7af430cfe315d6 100644
>> --- a/tools/testing/selftests/riscv/abi/.gitignore
>> +++ b/tools/testing/selftests/riscv/abi/.gitignore
>> @@ -1 +1,2 @@
>>  pointer_masking
>> +ptrace
>> diff --git a/tools/testing/selftests/riscv/abi/Makefile b/tools/testing/selftests/riscv/abi/Makefile
>> index ed82ff9c664e7eb3f760cbab81fb957ff72579c5..359a082c88a401883fb3776b35e4dacf69beaaaa 100644
>> --- a/tools/testing/selftests/riscv/abi/Makefile
>> +++ b/tools/testing/selftests/riscv/abi/Makefile
>> @@ -1,10 +1,14 @@
>>  # SPDX-License-Identifier: GPL-2.0
>>  
>>  CFLAGS += -I$(top_srcdir)/tools/include
>> +CFLAGS += $(KHDR_INCLUDES)
>>  
>> -TEST_GEN_PROGS := pointer_masking
>> +TEST_GEN_PROGS := pointer_masking ptrace
>>  
>>  include ../../lib.mk
>>  
>>  $(OUTPUT)/pointer_masking: pointer_masking.c
>>  	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
>> +
>> +$(OUTPUT)/ptrace: ptrace.c
>> +	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
>> diff --git a/tools/testing/selftests/riscv/abi/ptrace.c b/tools/testing/selftests/riscv/abi/ptrace.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..f1a0458adccdd040bfaa350e2e8d98b1ef34c0ad
>> --- /dev/null
>> +++ b/tools/testing/selftests/riscv/abi/ptrace.c
>> @@ -0,0 +1,201 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <unistd.h>
>> +#include <fcntl.h>
>> +#include <signal.h>
>> +#include <errno.h>
>> +#include <sys/types.h>
>> +#include <sys/ptrace.h>
>> +#include <sys/stat.h>
>> +#include <sys/user.h>
>> +#include <sys/wait.h>
>> +#include <sys/uio.h>
>> +#include <linux/elf.h>
>> +#include <linux/unistd.h>
>> +#include <linux/ptrace.h>
>> +#include <asm/ptrace.h>
>> +
>> +#include "../../kselftest_harness.h"
>> +
>> +#ifndef sizeof_field
>> +#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
>> +#endif
>> +#ifndef offsetofend
>> +#define offsetofend(TYPE, MEMBER) \
>> +	(offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
>> +#endif
> 
> I think this is the sixth test to define these. We should copy
> include/linux/stddef.h into tools/include. We already have
> tools/include/uapi/linux/stddef.h with __struct_group and
> __DECLARE_FLEX_ARRAY, so I think it should just work.

Agreed. But it may be better to be a separate patchset
so we can change those definition in different selftests
one pass.

> 
>> +
>> +
>> +#define ORIG_A0_MODIFY      0x01
>> +#define A0_MODIFY           0x02
>> +#define A0_OLD              0xbadbeefbeeff
>> +#define A0_NEW              0xffeebfeebdab
>> +
>> +
>> +struct a0_regs {
>> +	__s64 orig_a0;
>> +	__u64 a0;
>> +};
>> +
>> +#define perr_and_exit(fmt, ...)						\
>> +	({								\
>> +		char buf[256];						\
>> +		snprintf(buf, sizeof(buf), "%s:%d:" fmt ": %m\n",	\
>> +			__func__, __LINE__, ##__VA_ARGS__);		\
>> +		ksft_exit_fail_perror(buf);				\
>> +	})
>> +
>> +static void ptrace_test(int opt, struct a0_regs result[])
>> +{
>> +	int status;
>> +	long rc;
>> +	pid_t pid;
>> +	struct user_regs_struct regs;
>> +	struct iovec iov = {
>> +		.iov_base = &regs,
>> +		.iov_len = sizeof(regs),
>> +	};
>> +
>> +	unsigned long orig_a0;
>> +	struct iovec a0_iov = {
>> +		.iov_base = &orig_a0,
>> +		.iov_len = sizeof(orig_a0),
>> +	};
>> +	struct ptrace_syscall_info syscall_info = {
>> +		.op = 0xff,
>> +	};
>> +	const unsigned int expected_sci_entry_size =
>> +		offsetofend(struct ptrace_syscall_info, entry.args);
>> +	const unsigned int expected_sci_exit_size =
>> +		offsetofend(struct ptrace_syscall_info, exit.is_error);
>> +
>> +	pid = fork();
>> +	if (pid == 0) {
>> +		/* Mark oneself being traced */
>> +		long val = ptrace(PTRACE_TRACEME, 0, 0, 0);
>> +
>> +		if (val < 0)
>> +			perr_and_exit("failed to request for tracer to trace me: %ld", val);
>> +
>> +		kill(getpid(), SIGSTOP);
>> +
>> +		/* Perform chdir syscall that will be intercepted */
>> +		syscall(__NR_chdir, A0_OLD);
>> +
>> +		exit(0);
>> +	}
>> +
>> +	if (pid < 0)
>> +		ksft_exit_fail_perror("failed to fork");
>> +
>> +	for (int i = 0; i < 3; i++) {
>> +		if (waitpid(pid, &status, 0) != pid)
>> +			perr_and_exit("failed to wait for the tracee %d", pid);
>> +		if (WIFSTOPPED(status)) {
>> +			switch (WSTOPSIG(status)) {
>> +			case SIGSTOP:
>> +				if (ptrace(PTRACE_SETOPTIONS, pid, 0, PTRACE_O_TRACESYSGOOD) < 0)
>> +					perr_and_exit("failed to set PTRACE_O_TRACESYSGOOD");
>> +				break;
>> +			case SIGTRAP|0x80:
>> +				/* Modify twice so GET_SYSCALL_INFO get modified a0 and orig_a0 */
>> +				if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
>> +					perr_and_exit("failed to get tracee registers");
>> +				if (ptrace(PTRACE_GETREGSET, pid, NT_RISCV_ORIG_A0, &a0_iov))
>> +					perr_and_exit("failed to get tracee registers");
>> +
>> +				/* Modify a0/orig_a0 for the syscall */
>> +				switch (opt) {
>> +				case A0_MODIFY:
>> +					regs.a0 = A0_NEW;
>> +					break;
>> +				case ORIG_A0_MODIFY:
>> +					orig_a0 = A0_NEW;
>> +					break;
>> +				}
>> +
>> +				if (ptrace(PTRACE_SETREGSET, pid, NT_PRSTATUS, &iov))
>> +					perr_and_exit("failed to set tracee registers");
>> +				if (ptrace(PTRACE_SETREGSET, pid, NT_RISCV_ORIG_A0, &a0_iov))
>> +					perr_and_exit("failed to set tracee registers");
>> +				switch (i) {
>> +				case 1:
>> +					/* Stop at the beginning of syscall */
>> +					rc = ptrace(PTRACE_GET_SYSCALL_INFO, pid,
>> +						sizeof(syscall_info), &syscall_info);
>> +					if (rc < 0)
>> +						perr_and_exit("failed to get syscall info of entry");
>> +					if (rc < expected_sci_entry_size
>> +						|| syscall_info.op != PTRACE_SYSCALL_INFO_ENTRY)
>> +						perr_and_exit("stop position of entry mismatched");
>> +					result[0].orig_a0 = syscall_info.entry.args[0];
>> +					break;
>> +
>> +				case 2:
>> +					/* Stop at the end of syscall */
>> +					rc = ptrace(PTRACE_GET_SYSCALL_INFO, pid,
>> +						sizeof(syscall_info), &syscall_info);
>> +					if (rc < 0)
>> +						perr_and_exit("failed to get syscall info of entry");
>> +					if (rc < expected_sci_exit_size
>> +						|| syscall_info.op != PTRACE_SYSCALL_INFO_EXIT)
>> +						perr_and_exit("stop position of exit mismatched");
>> +					result[0].a0 = syscall_info.exit.rval;
>> +
>> +					if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
>> +						perr_and_exit("failed to get tracee registers");
>> +					result[1].a0 = regs.a0;
>> +					if (ptrace(PTRACE_GETREGSET, pid, NT_RISCV_ORIG_A0,
>> +						   &a0_iov))
>> +						perr_and_exit("failed to get tracee registers");
>> +					result[1].orig_a0 = orig_a0;
>> +				}
>> +			}
>> +			if (ptrace(PTRACE_SYSCALL, pid, 0, 0) < 0)
>> +				perr_and_exit("failed to resume tracee");
>> +		}
>> +	}
>> +
>> +	/* Resume the tracee */
>> +	ptrace(PTRACE_CONT, pid, 0, 0);
>> +	if (waitpid(pid, &status, 0) != pid)
>> +		perr_and_exit("failed to wait for the tracee");
>> +
>> +}
>> +
>> +TEST(ptrace_access_a0)
>> +{
>> +	struct a0_regs result[2];
>> +
>> +	ptrace_test(A0_MODIFY, result);
>> +
>> +	/* Verify PTRACE_SETREGSET */
>> +	/* The modification of a0 cannot affect the first argument of the syscall */
>> +	EXPECT_EQ(A0_OLD, result[0].orig_a0);
>> +	EXPECT_EQ(A0_NEW, result[0].a0);
>> +
>> +	/* Verify PTRACE_GETREGSET */
>> +	EXPECT_EQ(result[1].orig_a0, result[0].orig_a0);
>> +	EXPECT_EQ(result[1].a0, result[0].a0);
>> +}
>> +
>> +TEST(ptrace_access_orig_a0)
>> +{
>> +	struct a0_regs result[2];
>> +
>> +	ptrace_test(ORIG_A0_MODIFY, result);
>> +
>> +	/* Verify PTRACE_SETREGSET */
>> +	/* Only modify orig_a0 to change the first argument of the syscall */
>> +	EXPECT_EQ(A0_NEW, result[0].orig_a0);
>> +	/* a0 will keep default value, orig_a0 or -ENOSYS, depends on internal. */
>> +	EXPECT_NE(A0_NEW, result[0].a0);
> 
> I don't understand this test. Why don't we know what to expect? Also, the
> comment says orig_a0 is an option for the value, but then we don't expect
> it to be A0_NEW, even though we expect orig_a0 to be A0_NEW?

The purpose of the test is to ensure that the ORIG_A0_MODIFY operation
will not affect the a0 register (So it will not be our A0_NEW). But there
is a problem with the comment. It is written for some old wrong code.
I will correct the comment.

> 
>> +
>> +	/* Verify PTRACE_GETREGSET */
>> +	EXPECT_EQ(result[1].orig_a0, result[0].orig_a0);
>> +	EXPECT_EQ(result[1].a0, result[0].a0);
>> +}
>> +
>> +TEST_HARNESS_MAIN
>>
>> -- 
>> 2.48.0
>>
> 
> Other than the two comments/questions, this test looks great.
> 
> Thanks,
> drew
Andrew Jones Jan. 15, 2025, 9:56 a.m. UTC | #3
On Wed, Jan 15, 2025 at 05:41:57PM +0800, Celeste Liu wrote:
> On 2025-01-15 17:14, Andrew Jones wrote:
> > On Wed, Jan 15, 2025 at 04:24:59AM +0800, Celeste Liu wrote:
...
> >> +#ifndef sizeof_field
> >> +#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
> >> +#endif
> >> +#ifndef offsetofend
> >> +#define offsetofend(TYPE, MEMBER) \
> >> +	(offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
> >> +#endif
> > 
> > I think this is the sixth test to define these. We should copy
> > include/linux/stddef.h into tools/include. We already have
> > tools/include/uapi/linux/stddef.h with __struct_group and
> > __DECLARE_FLEX_ARRAY, so I think it should just work.
> 
> Agreed. But it may be better to be a separate patchset
> so we can change those definition in different selftests
> one pass.
>

I think a separate "copy stddef.h" patch could be in this series to
avoid having to add the defines here. Then, another series can be
sent with one patch for each conversion. That said, I'm OK with
adding the defines for now and doing the conversion later. I just
hope it will actually happen.

Thanks,
drew
Celeste Liu Jan. 15, 2025, 11:23 a.m. UTC | #4
On 2025-01-15 17:56, Andrew Jones wrote:
> On Wed, Jan 15, 2025 at 05:41:57PM +0800, Celeste Liu wrote:
>> On 2025-01-15 17:14, Andrew Jones wrote:
>>> On Wed, Jan 15, 2025 at 04:24:59AM +0800, Celeste Liu wrote:
> ...
>>>> +#ifndef sizeof_field
>>>> +#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
>>>> +#endif
>>>> +#ifndef offsetofend
>>>> +#define offsetofend(TYPE, MEMBER) \
>>>> +	(offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
>>>> +#endif
>>>
>>> I think this is the sixth test to define these. We should copy
>>> include/linux/stddef.h into tools/include. We already have
>>> tools/include/uapi/linux/stddef.h with __struct_group and
>>> __DECLARE_FLEX_ARRAY, so I think it should just work.
>>
>> Agreed. But it may be better to be a separate patchset
>> so we can change those definition in different selftests
>> one pass.
>>
> 
> I think a separate "copy stddef.h" patch could be in this series to
> avoid having to add the defines here. Then, another series can be
> sent with one patch for each conversion. That said, I'm OK with
> adding the defines for now and doing the conversion later. I just
> hope it will actually happen.

v6 has been sent. The "copy stddef.h" patch has been included.
Once this patchset land, I will send another patchset to deal
with other usages.

https://lore.kernel.org/lkml/20250115-riscv-new-regset-v6-0-59bfddd33525@coelacanthus.name/T/

> 
> Thanks,
> drew
diff mbox series

Patch

diff --git a/tools/testing/selftests/riscv/abi/.gitignore b/tools/testing/selftests/riscv/abi/.gitignore
index b38358f91c4d2240ae64892871d9ca98bda1ae58..378c605919a3b3d58eec2701eb7af430cfe315d6 100644
--- a/tools/testing/selftests/riscv/abi/.gitignore
+++ b/tools/testing/selftests/riscv/abi/.gitignore
@@ -1 +1,2 @@ 
 pointer_masking
+ptrace
diff --git a/tools/testing/selftests/riscv/abi/Makefile b/tools/testing/selftests/riscv/abi/Makefile
index ed82ff9c664e7eb3f760cbab81fb957ff72579c5..359a082c88a401883fb3776b35e4dacf69beaaaa 100644
--- a/tools/testing/selftests/riscv/abi/Makefile
+++ b/tools/testing/selftests/riscv/abi/Makefile
@@ -1,10 +1,14 @@ 
 # SPDX-License-Identifier: GPL-2.0
 
 CFLAGS += -I$(top_srcdir)/tools/include
+CFLAGS += $(KHDR_INCLUDES)
 
-TEST_GEN_PROGS := pointer_masking
+TEST_GEN_PROGS := pointer_masking ptrace
 
 include ../../lib.mk
 
 $(OUTPUT)/pointer_masking: pointer_masking.c
 	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
+
+$(OUTPUT)/ptrace: ptrace.c
+	$(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^
diff --git a/tools/testing/selftests/riscv/abi/ptrace.c b/tools/testing/selftests/riscv/abi/ptrace.c
new file mode 100644
index 0000000000000000000000000000000000000000..f1a0458adccdd040bfaa350e2e8d98b1ef34c0ad
--- /dev/null
+++ b/tools/testing/selftests/riscv/abi/ptrace.c
@@ -0,0 +1,201 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <signal.h>
+#include <errno.h>
+#include <sys/types.h>
+#include <sys/ptrace.h>
+#include <sys/stat.h>
+#include <sys/user.h>
+#include <sys/wait.h>
+#include <sys/uio.h>
+#include <linux/elf.h>
+#include <linux/unistd.h>
+#include <linux/ptrace.h>
+#include <asm/ptrace.h>
+
+#include "../../kselftest_harness.h"
+
+#ifndef sizeof_field
+#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
+#endif
+#ifndef offsetofend
+#define offsetofend(TYPE, MEMBER) \
+	(offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
+#endif
+
+
+#define ORIG_A0_MODIFY      0x01
+#define A0_MODIFY           0x02
+#define A0_OLD              0xbadbeefbeeff
+#define A0_NEW              0xffeebfeebdab
+
+
+struct a0_regs {
+	__s64 orig_a0;
+	__u64 a0;
+};
+
+#define perr_and_exit(fmt, ...)						\
+	({								\
+		char buf[256];						\
+		snprintf(buf, sizeof(buf), "%s:%d:" fmt ": %m\n",	\
+			__func__, __LINE__, ##__VA_ARGS__);		\
+		ksft_exit_fail_perror(buf);				\
+	})
+
+static void ptrace_test(int opt, struct a0_regs result[])
+{
+	int status;
+	long rc;
+	pid_t pid;
+	struct user_regs_struct regs;
+	struct iovec iov = {
+		.iov_base = &regs,
+		.iov_len = sizeof(regs),
+	};
+
+	unsigned long orig_a0;
+	struct iovec a0_iov = {
+		.iov_base = &orig_a0,
+		.iov_len = sizeof(orig_a0),
+	};
+	struct ptrace_syscall_info syscall_info = {
+		.op = 0xff,
+	};
+	const unsigned int expected_sci_entry_size =
+		offsetofend(struct ptrace_syscall_info, entry.args);
+	const unsigned int expected_sci_exit_size =
+		offsetofend(struct ptrace_syscall_info, exit.is_error);
+
+	pid = fork();
+	if (pid == 0) {
+		/* Mark oneself being traced */
+		long val = ptrace(PTRACE_TRACEME, 0, 0, 0);
+
+		if (val < 0)
+			perr_and_exit("failed to request for tracer to trace me: %ld", val);
+
+		kill(getpid(), SIGSTOP);
+
+		/* Perform chdir syscall that will be intercepted */
+		syscall(__NR_chdir, A0_OLD);
+
+		exit(0);
+	}
+
+	if (pid < 0)
+		ksft_exit_fail_perror("failed to fork");
+
+	for (int i = 0; i < 3; i++) {
+		if (waitpid(pid, &status, 0) != pid)
+			perr_and_exit("failed to wait for the tracee %d", pid);
+		if (WIFSTOPPED(status)) {
+			switch (WSTOPSIG(status)) {
+			case SIGSTOP:
+				if (ptrace(PTRACE_SETOPTIONS, pid, 0, PTRACE_O_TRACESYSGOOD) < 0)
+					perr_and_exit("failed to set PTRACE_O_TRACESYSGOOD");
+				break;
+			case SIGTRAP|0x80:
+				/* Modify twice so GET_SYSCALL_INFO get modified a0 and orig_a0 */
+				if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
+					perr_and_exit("failed to get tracee registers");
+				if (ptrace(PTRACE_GETREGSET, pid, NT_RISCV_ORIG_A0, &a0_iov))
+					perr_and_exit("failed to get tracee registers");
+
+				/* Modify a0/orig_a0 for the syscall */
+				switch (opt) {
+				case A0_MODIFY:
+					regs.a0 = A0_NEW;
+					break;
+				case ORIG_A0_MODIFY:
+					orig_a0 = A0_NEW;
+					break;
+				}
+
+				if (ptrace(PTRACE_SETREGSET, pid, NT_PRSTATUS, &iov))
+					perr_and_exit("failed to set tracee registers");
+				if (ptrace(PTRACE_SETREGSET, pid, NT_RISCV_ORIG_A0, &a0_iov))
+					perr_and_exit("failed to set tracee registers");
+				switch (i) {
+				case 1:
+					/* Stop at the beginning of syscall */
+					rc = ptrace(PTRACE_GET_SYSCALL_INFO, pid,
+						sizeof(syscall_info), &syscall_info);
+					if (rc < 0)
+						perr_and_exit("failed to get syscall info of entry");
+					if (rc < expected_sci_entry_size
+						|| syscall_info.op != PTRACE_SYSCALL_INFO_ENTRY)
+						perr_and_exit("stop position of entry mismatched");
+					result[0].orig_a0 = syscall_info.entry.args[0];
+					break;
+
+				case 2:
+					/* Stop at the end of syscall */
+					rc = ptrace(PTRACE_GET_SYSCALL_INFO, pid,
+						sizeof(syscall_info), &syscall_info);
+					if (rc < 0)
+						perr_and_exit("failed to get syscall info of entry");
+					if (rc < expected_sci_exit_size
+						|| syscall_info.op != PTRACE_SYSCALL_INFO_EXIT)
+						perr_and_exit("stop position of exit mismatched");
+					result[0].a0 = syscall_info.exit.rval;
+
+					if (ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov))
+						perr_and_exit("failed to get tracee registers");
+					result[1].a0 = regs.a0;
+					if (ptrace(PTRACE_GETREGSET, pid, NT_RISCV_ORIG_A0,
+						   &a0_iov))
+						perr_and_exit("failed to get tracee registers");
+					result[1].orig_a0 = orig_a0;
+				}
+			}
+			if (ptrace(PTRACE_SYSCALL, pid, 0, 0) < 0)
+				perr_and_exit("failed to resume tracee");
+		}
+	}
+
+	/* Resume the tracee */
+	ptrace(PTRACE_CONT, pid, 0, 0);
+	if (waitpid(pid, &status, 0) != pid)
+		perr_and_exit("failed to wait for the tracee");
+
+}
+
+TEST(ptrace_access_a0)
+{
+	struct a0_regs result[2];
+
+	ptrace_test(A0_MODIFY, result);
+
+	/* Verify PTRACE_SETREGSET */
+	/* The modification of a0 cannot affect the first argument of the syscall */
+	EXPECT_EQ(A0_OLD, result[0].orig_a0);
+	EXPECT_EQ(A0_NEW, result[0].a0);
+
+	/* Verify PTRACE_GETREGSET */
+	EXPECT_EQ(result[1].orig_a0, result[0].orig_a0);
+	EXPECT_EQ(result[1].a0, result[0].a0);
+}
+
+TEST(ptrace_access_orig_a0)
+{
+	struct a0_regs result[2];
+
+	ptrace_test(ORIG_A0_MODIFY, result);
+
+	/* Verify PTRACE_SETREGSET */
+	/* Only modify orig_a0 to change the first argument of the syscall */
+	EXPECT_EQ(A0_NEW, result[0].orig_a0);
+	/* a0 will keep default value, orig_a0 or -ENOSYS, depends on internal. */
+	EXPECT_NE(A0_NEW, result[0].a0);
+
+	/* Verify PTRACE_GETREGSET */
+	EXPECT_EQ(result[1].orig_a0, result[0].orig_a0);
+	EXPECT_EQ(result[1].a0, result[0].a0);
+}
+
+TEST_HARNESS_MAIN