From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from vmicros1.altlinux.org (vmicros1.altlinux.org [194.107.17.57]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 48ACD31CA4E for ; Wed, 8 Jul 2026 07:47:31 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=194.107.17.57 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783496861; cv=none; b=qinF+RTzdz6JgsFUtJINMvetuxbTEG45dZ6qBin2/A5CqZjUcu6MQ4mZzAdYxQRZo/NbVYCUn0lYM5qQ99BuhbDqnXM+C6+GdAsUpgFop5a85ar/FXwPBI8zkEmELy/TnWIilV2rhpF89TgtwYpIpjnacD/aNf4MkQlw5WJ8C8E= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783496861; c=relaxed/simple; bh=l/2l6m9dq1j1WbzuA037oVeea2SmyNrIUZyuwBZCIi8=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=HVhz/oP11OvEMeoHIICFy8sQ+as671FAtB+6K9QrFBkGnrOp/t5nNZz7ojlIUCqNEIxE6GRWoAatdoSt1w6eov1RXoGU2IATuZ4v7Clz6Vkqokg0mjpumGUFEbJwTX12TF1YxrbXgyuLcQW0WCxDzX5HiZgpfxN7t4emo3wg8qE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=strace.io; spf=pass smtp.mailfrom=altlinux.org; arc=none smtp.client-ip=194.107.17.57 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=strace.io Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=altlinux.org Received: from mua.local.altlinux.org (mua.local.altlinux.org [192.168.1.14]) by vmicros1.altlinux.org (Postfix) with ESMTP id A244672C8CC; Wed, 8 Jul 2026 10:47:26 +0300 (MSK) Received: by mua.local.altlinux.org (Postfix, from userid 508) id 904EE7CCB3A; Wed, 8 Jul 2026 10:47:26 +0300 (IDT) Date: Wed, 8 Jul 2026 10:47:26 +0300 From: "Dmitry V. Levin" To: Renzo Davoli Cc: linux-kernel@vger.kernel.org, Andrew Morton , Oleg Nesterov , Shuah Khan , Alexey Gladkov , Eugene Syromyatnikov , Mike Frysinger , Davide Berardi , strace-devel@lists.strace.io Subject: Re: [PATCH v3 2/2] selftests/ptrace: add a test case for PTRACE_SYSCALL_INFO syscall skipping Message-ID: <20260708074726.GA27940@strace.io> References: <20260707112107.920752-1-renzo@cs.unibo.it> <20260707112107.920752-3-renzo@cs.unibo.it> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260707112107.920752-3-renzo@cs.unibo.it> On Tue, Jul 07, 2026 at 01:21:07PM +0200, Renzo Davoli wrote: > Check whether PTRACE_SYSCALL_INFO syscall skiping semantics implemented in the > kernel matches userspace expectations. typo: skiping > > Signed-off-by: Renzo Davoli > --- > .../selftests/ptrace/set_syscall_info.c | 179 +++++++++++++++++- > 1 file changed, 178 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/ptrace/set_syscall_info.c b/tools/testing/selftests/ptrace/set_syscall_info.c > index 1cc411a41cd6..af3c908b0d68 100644 > --- a/tools/testing/selftests/ptrace/set_syscall_info.c > +++ b/tools/testing/selftests/ptrace/set_syscall_info.c > @@ -11,9 +11,16 @@ > #include > #include > #include > +#include > +#include > #include > +#include > #include > #include > +#include > +#include > +#include > + Even though an extra blank line is not a problem, it's a little distraction from the change. > #if defined(_MIPS_SIM) && _MIPS_SIM == _MIPS_SIM_NABI32 > /* > @@ -36,6 +43,7 @@ struct si_exit { > > static unsigned int ptrace_stop; > static pid_t tracee_pid; > +static pid_t tracer_pid; > > static int > kill_tracee(pid_t pid) > @@ -64,6 +72,25 @@ sys_ptrace(int request, pid_t pid, unsigned long addr, unsigned long data) > ptrace_stop, ##__VA_ARGS__); \ > } while (0) > > +static int sys_seccomp(unsigned int operation, unsigned int flags, void *args) > +{ > + return syscall(__NR_seccomp, operation, flags, args); > +} > + > +static struct sock_filter seccomp_filter[] = { > + BPF_STMT(BPF_LD+BPF_W+BPF_ABS, offsetof(struct seccomp_data, nr)), > + > + BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, __NR_restart_syscall, 0, 1), > + BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_ALLOW), > + > + BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_TRACE), > +}; > + > +static struct sock_fprog seccomp_prog = { > + .filter = seccomp_filter, > + .len = ARRAY_SIZE(seccomp_filter) > +}; > + > static void > check_psi_entry(struct __test_metadata *_metadata, > const struct ptrace_syscall_info *info, > @@ -128,7 +155,6 @@ check_psi_exit(struct __test_metadata *_metadata, > > TEST(set_syscall_info) > { > - const pid_t tracer_pid = getpid(); > const kernel_ulong_t dummy[] = { > (kernel_ulong_t) 0xdad0bef0bad0fed0ULL, > (kernel_ulong_t) 0xdad1bef1bad1fed1ULL, > @@ -138,6 +164,7 @@ TEST(set_syscall_info) > (kernel_ulong_t) 0xdad5bef5bad5fed5ULL, > }; > int splice_in[2], splice_out[2]; > + tracer_pid = getpid(); > > ASSERT_EQ(0, pipe(splice_in)); > ASSERT_EQ(0, pipe(splice_out)); > @@ -516,4 +543,154 @@ TEST(set_syscall_info) > ASSERT_EQ(ptrace_stop, ARRAY_SIZE(si) * 2); > } > > +TEST(set_syscall_info_seccomp) > +{ > + tracer_pid = getpid(); > + tracee_pid = fork(); > + > + ASSERT_LE(0, tracee_pid) { > + TH_LOG("fork: %m"); > + } > + > + /* tracee */ > + if (tracee_pid == 0) { > + tracee_pid = getpid(); > + ASSERT_EQ(0, sys_ptrace(PTRACE_TRACEME, 0, 0, 0)) { > + TH_LOG("PTRACE_TRACEME: %m"); > + } > + ASSERT_EQ(0, kill(tracee_pid, SIGSTOP)) { > + /* cannot happen */ > + TH_LOG("kill SIGSTOP: %m"); > + } > + > + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) { > + TH_LOG("prctl: %m"); > + _exit(1); > + } > + ASSERT_EQ(0, sys_seccomp(SECCOMP_SET_MODE_FILTER, 0, > + (void *) &seccomp_prog)) { > + TH_LOG("seccomp: %m"); > + _exit(1); > + } > + > + /* run getpid unmodified */ > + ASSERT_EQ(tracee_pid, getpid()) { > + TH_LOG("getpid seccomp unchanged: %m"); > + _exit(1); > + } > + > + /* run getppid instead of getpid */ > + ASSERT_EQ(tracer_pid, getpid()) { > + TH_LOG("getpid seccomp nr changes: %m"); > + _exit(1); > + } > + > + /* skip getpid and return 42 */ > + ASSERT_EQ(42, getpid()) { > + TH_LOG("getpid skip set return value changes: %m"); > + _exit(1); > + } Since ASSERT_EQ aborts in case of assertion failure, why do you need these explicit _exit(1) calls here? > + _exit(0); > + } > + > + int status; > + > + /* tracer */ > + ASSERT_LE(0, waitpid(-1,&status,0)) { Missing spaces after comma: waitpid(-1,&status,0) -> waitpid(-1, &status, 0) > + LOG_KILL_TRACEE("waitpid: %m"); > + } > + > + ASSERT_EQ(0, sys_ptrace(PTRACE_SETOPTIONS, tracee_pid, 0, PTRACE_O_TRACESECCOMP | PTRACE_O_TRACESYSGOOD)) > + LOG_KILL_TRACEE("PTRACE_SETOPTIONS: %m"); Missing braces on ASSERT_EQ. > + > + ASSERT_EQ(0, sys_ptrace(PTRACE_CONT, tracee_pid, 0, 0)) { > + LOG_KILL_TRACEE("PTRACE_CONT: %m"); > + } > + > + while (1) { I suggest to replace while (1) { with for (ptrace_stop = 0; ; ++ptrace_stop) { and don't bother changing ptrace_stop inside the loop. > + ASSERT_EQ(tracee_pid, wait(&status)) { > + /* cannot happen */ > + LOG_KILL_TRACEE("wait: %m"); > + } > + if (WIFEXITED(status)) { > + tracee_pid = 0; /* the tracee is no more */ > + ASSERT_EQ(0, WEXITSTATUS(status)) { > + LOG_KILL_TRACEE("unexpected exit status %u", > + WEXITSTATUS(status)); > + } > + break; > + } > + ASSERT_FALSE(WIFSIGNALED(status)) { > + tracee_pid = 0; /* the tracee is no more */ > + LOG_KILL_TRACEE("unexpected signal %u", > + WTERMSIG(status)); > + } > + ASSERT_TRUE(WIFSTOPPED(status)) { > + LOG_KILL_TRACEE("unexpected wait status %#x", status); > + } > + > + if (status >> 8 == (SIGTRAP | (PTRACE_EVENT_SECCOMP << 8))) { I suggest turning this into assertion, e.g. ASSERT_EQ(status >> 8, SIGTRAP | (PTRACE_EVENT_SECCOMP << 8)) { LOG_KILL_TRACEE("unexpected stop, wait status %#x", status); } > + struct ptrace_syscall_info info = { > + .op = 0xff /* invalid PTRACE_SYSCALL_INFO_* op */ > + }; > + size_t info_size = sizeof(info); > + > + ASSERT_LT(0, sys_ptrace(PTRACE_GET_SYSCALL_INFO, tracee_pid, info_size, (uintptr_t) &info)) { > + LOG_KILL_TRACEE("PTRACE_GET_SYSCALL_INFO: %m"); > + }; Stray semicolon after the closing brace. > + ASSERT_EQ(PTRACE_SYSCALL_INFO_SECCOMP, info.op) { > + LOG_KILL_TRACEE("entry op mismatch: %m"); > + } > + ASSERT_TRUE(info.arch) { > + LOG_KILL_TRACEE("entry arch mismatch: %m"); > + } > + ASSERT_TRUE(info.instruction_pointer) { > + LOG_KILL_TRACEE("entry instruction_pointer mismatch: %m"); > + } > + ASSERT_TRUE(info.stack_pointer) { > + LOG_KILL_TRACEE("entry stack_pointer mismatch: %m"); > + } I suggest adding an extra check here: ASSERT_LT(ptrace_stop, 4) { LOG_KILL_TRACEE("ptrace stop overflow"); } > + switch (ptrace_stop) { > + case 0: ASSERT_EQ(__NR_getpid, info.seccomp.nr) { Trailing statements on case lines: "case 0: ASSERT_EQ(...)" should have the ASSERT on the next line. > + LOG_KILL_TRACEE("step %d nr __NR_getpid mismatch: %m", ptrace_stop); As LOG_KILL_TRACEE() prints ptrace_stop itself, let's keep it simple, e.g. LOG_KILL_TRACEE("syscall nr mismatch"); > + } > + ptrace_stop++; > + break; > + case 1: ASSERT_EQ(__NR_getpid, info.seccomp.nr) { > + LOG_KILL_TRACEE("step %d nr __NR_getpid mismatch: %m", ptrace_stop); > + } > + info.seccomp.nr = __NR_getppid; > + ptrace_stop++; > + break; > + case 2: ASSERT_EQ(__NR_getpid, info.seccomp.nr) { > + LOG_KILL_TRACEE("step %d nr __NR_getpid mismatch: %m", ptrace_stop); > + } > + info.op = PTRACE_SYSCALL_INFO_EXIT; > + info.exit.rval = 42; > + info.exit.is_error = 0; > + ptrace_stop++; > + break; > + case 3: ASSERT_EQ(__NR_exit_group, info.seccomp.nr) { > + LOG_KILL_TRACEE("step %d nr __NR_exit_group mismatch: %m", ptrace_stop); > + } > + break; > + default: > + LOG_KILL_TRACEE("unexpected system call: %m"); > + break; > + > + } Given that all info.seccomp.nr assertions are essentially identical, this could be simplified into something like this: const unsigned int expected_nr[] = { __NR_getpid, __NR_getpid, __NR_getpid, __NR_exit_group }; ASSERT_LT(ptrace_stop, ARRAY_SIZE(expected_nr)) { LOG_KILL_TRACEE("ptrace stop overflow"); } ASSERT_EQ(info.seccomp.nr, expected_nr[ptrace_stop]) { LOG_KILL_TRACEE("syscall nr mismatch"); } switch (ptrace_stop) { case 0: case 3: break; case 1: info.seccomp.nr = __NR_getppid; break; case 2: info.op = PTRACE_SYSCALL_INFO_EXIT; info.exit.rval = 42; info.exit.is_error = 0; break; } > + ASSERT_EQ(0,sys_ptrace(PTRACE_SET_SYSCALL_INFO, tracee_pid, info_size, (uintptr_t) &info)) { Missing space after comma. > + LOG_KILL_TRACEE("PTRACE_SET_SYSCALL_INFO: %m"); > + } > + > + ASSERT_EQ(0,sys_ptrace(PTRACE_CONT, tracee_pid, 0, 0)) { Likewise. -- ldv