From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 84F2A3019D6 for ; Sat, 4 Apr 2026 20:13:32 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775333612; cv=none; b=teIZbtdiLXQIwGBQf9cnYMX/F+vpUb8WPTZxbsyj8QLAYT6p026T4LcB02e8bYjlDiYUQiTATCUiHQGZL4XuiTpasZx6GlBMZwUQZvavHGMAFSmEkPQV4y4S06HnYM+6uFLdWERA1JfnNOTHN5JwYVy3E1+ph67snxwS7x0/+UE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775333612; c=relaxed/simple; bh=8RcPBie7tDakVbReyfQwoYDUA1iwLUol7d/IQR0widQ=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=ubLP4jW/a4Q9jlXkkHTv3HqEUCFxxEjM4YRkqsmiJEGuUh65BT6tFEplPZpo+tXJ1c4g13Bts5viB0RkSD8PdHOQs2Sle//w6LE+Ieeia3KVGgppFALSTNhPK8k0kuBf8isDEljQ8JhCzgBrNMH4qwu7w60Rq7ZEKM6fm2Y2vb8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=YoNQyrck; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="YoNQyrck" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 38BDDC19421; Sat, 4 Apr 2026 20:13:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1775333612; bh=8RcPBie7tDakVbReyfQwoYDUA1iwLUol7d/IQR0widQ=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=YoNQyrckrDjkuaQpoxoC32RMsTyrxs5RO7gnL1mEM8GkF4hEEYdEsek/JBhO5CUE2 pe1xvUQOnLcpKg5qD6w2c89kJK3RcHDp5icDjK3gqOPsmTcctxWvBWT1hlEvq1QKLb pFne9IQVMrBSu9aNn+dtMwVSVzy8THDEF8r2EQAu7pur87dFj7aOYAzEqND1IGLdbx 9V6vLv5R7KQWntR834C3yYhSqRkLgn7pJiNK05QOtdYJpSKDgNzSsETGoP56qQb2J9 tWSyi0/ey7twBADMZkehPDHXXIVW2k0gD3l5HDSe/JrPfjRnknHOMZtKJIeEZWplqs pGDFOXeHKJi0A== From: Thomas Gleixner To: Sebastian Andrzej Siewior Cc: LKML , Mathieu Desnoyers , =?utf-8?Q?Andr=C3=A8?= Almeida , Carlos O'Donell , Peter Zijlstra , Florian Weimer , Rich Felker , Torvald Riegel , Darren Hart , Ingo Molnar , Davidlohr Bueso , Arnd Bergmann , "Liam R . Howlett" , Uros Bizjak , Thomas =?utf-8?Q?Wei=C3=9Fschuh?= Subject: Re: [PATCH 15/14] selftests: futex: Add tests for robust unlock within the critical section. In-Reply-To: <20260404093939.7XgeW_54@linutronix.de> References: <20260402151131.876492985@kernel.org> <20260404093939.7XgeW_54@linutronix.de> Date: Sat, 04 Apr 2026 22:13:28 +0200 Message-ID: <87mrzi5vrr.ffs@tglx> 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=utf-8 Content-Transfer-Encoding: quoted-printable On Sat, Apr 04 2026 at 11:39, Sebastian Andrzej Siewior wrote: > From: Sebastian Andrzej Siewior > > I took Thomas=E2=80=99 initial test case from the cover letter and rework= ed it > so that it uses ptrace() to single=E2=80=91step through the VDSO unlock > operation. The test expects the lock to remain locked, with > `list_op_pending' pointing somewhere, when entering the VDSO unlock > path. Once execution steps into the critical section, it expects the > kernel to perform the fixup that is, to unlock the lock and clear > `list_op_pending'. No. The kernel clears list_op_pending when the unlock via CMPXCHG was successful. The unlock and clear combo happens in the syscall with the UNLOCK modifier set. > +static char *vdso_dbg_file =3D > +#ifdef __LP64__ > + "vdso64.so.dbg"; > +#else > + "vdso32.so.dbg"; > +#endif You wish. There is a zoo of names for the VDSOs and not all architectures do the vdso64/32 scheme. > + > +static bool pc_is_within(struct user_regs_struct *regs, uint64_t start, = uint64_t end) > +{ > + unsigned long pc; > + > +#if defined(__x86_64__) > + pc =3D regs->rip; > +#elif defined(__riscv) > + pc =3D reg->pc; > +# error Missing ptrace support #else # error not supported > +#endif > + if (pc >=3D (long) start && pc < end) > + return true; > + return false; return pc >=3D start && pc < end; perhaps? > +static int is_vdso_mod(const char *mod) > +{ > + return !strncmp(mod, "[vdso: ", 7); > +} > + > +static int module_callback(Dwfl_Module *mod, void **userdata, const char= *name, > + Dwarf_Addr start, void *arg) > +{ > + const char *sname; Snip. > + if (dwfl_getmodules(dwfl, module_callback, NULL, 0) !=3D 0) > + err (1, "dwfl_getmodules: %s", dwfl_errmsg(-1)); > + dwfl_end(dwfl); > +} That's a lot of complicated code. Wouldn't it be simpler to have a wrapper script which finds the vdso64/32.so.dbg for the system and gets the offsets via objdump -t and hands them in as command line arguments? Then you can use the simple function lookup from patch 14. No strong preference though. Just a thought. > + > +static pthread_mutex_t test_mutex; Newline between the variable and the code please. =20 > +static int unlock_uncontended(struct __test_metadata *_metadata, bool is= _32bit) > +{ > + struct robust_list_head *rhead; > + uint32_t *lock; > + pid_t lock_tid; > + int error =3D 1; > + pid_t tid; > + size_t sz; > + > + syscall(SYS_get_robust_list, 0, &rhead, &sz); Can fail. > + pthread_mutex_init(&test_mutex, NULL); > + > + tid =3D gettid(); > + lock =3D (uint32_t *)&test_mutex.__data.__lock; > + *lock =3D tid; > + /* Set the pending prior unlocking */ > + rhead->list_op_pending =3D (struct robust_list *)&test_mutex.__data.__l= ist.__next; > + > + raise(SIGTRAP); > + if (is_32bit) > + lock_tid =3D frtu32(lock, tid, (uint32_t *)&rhead->list_op_pending); > + else > + lock_tid =3D frtu64(lock, tid, (uint64_t *)&rhead->list_op_pending); > + > + if (lock_tid !=3D tid) > + TH_LOG("Non contended unlock failed. Return: %08x expected %08x\n", > + lock_tid, tid); > + else > + error =3D 0; > + return error; > +} > + > +enum trace_state { > + STATE_WAIT =3D 0, > + STATE_ENTER_VDSO, > + STATE_IN_CS, > + STATE_OUT_CS, > + STATE_LEAVE_VDSO, > +}; > + > +static void trace_child(struct __test_metadata *_metadata, pid_t child, = bool is_32bit) > +{ > + int state =3D STATE_WAIT; > + struct robust_list_head *rhead; > + size_t sz; > + bool do_end =3D false; Variable ordering reverse fir tree please. > + syscall(SYS_get_robust_list, 0, &rhead, &sz); Can fail. > + do { > + struct user_regs_struct regs; > + uint64_t lock_val; > + bool in_vdso; > + int wstatus; > + pid_t rpid; > + > + rpid =3D waitpid(child, &wstatus, 0); > + if (rpid !=3D child) > + errx(1, "waitpid"); > + if (!do_end) { > + if (WSTOPSIG(wstatus) !=3D SIGTRAP) > + errx(1, "NOT SIGTRAP"); > + } else { > + if (!WIFEXITED(wstatus)) > + errx(1, "Did not exit, but we are done"); > + ASSERT_EQ(WEXITSTATUS(wstatus), 0); > + return; > + } > + > + if (ptrace(PTRACE_GETREGS, child, 0, ®s) !=3D 0) > + errx(1, "PTRACE_GETREGS"); > + > + if (is_32bit) { > + in_vdso =3D pc_is_within(®s, (long)frtu32, frtu32_end); > + } else { > + in_vdso =3D pc_is_within(®s, (long)frtu64, frtu64_end); > + } No brackets required. > + if (in_vdso) { > + unsigned long rhead_val; > + > + if (state =3D=3D STATE_WAIT) { > + state =3D STATE_ENTER_VDSO; > + > + } else { > + if (is_32bit) { > + if (pc_is_within(®s, __futex_list32_try_unlock_cs_start, > + __futex_list32_try_unlock_cs_end)) > + state =3D STATE_IN_CS; > + else > + state =3D STATE_OUT_CS; > + } else { > + if (pc_is_within(®s, __futex_list64_try_unlock_cs_start, > + __futex_list64_try_unlock_cs_end)) > + state =3D STATE_IN_CS; > + else > + state =3D STATE_OUT_CS; > + } > + } See below. > + errno =3D 0; > + rhead_val =3D ptrace(PTRACE_PEEKDATA, child, &rhead->list_op_pending,= 0); > + if (rhead_val =3D=3D -1 && errno !=3D 0) > + err(1, "PTRACE_PEEKDATA"); > + > + lock_val =3D ptrace(PTRACE_PEEKDATA, child, &test_mutex.__data.__lock= , 0); > + if (lock_val =3D=3D -1 && errno !=3D 0) > + err(1, "PTRACE_PEEKDATA"); > + > + if (state =3D=3D STATE_ENTER_VDSO) { > + /* Entering VDSO, it supposed to be locked */ > + ASSERT_NE(rhead_val, 0); > + ASSERT_EQ(lock_val, child); > + > + } else if (state =3D=3D STATE_IN_CS) { > + /* > + * If the critical section has been entered then > + * the kernel has to unlock and clean list_op_pending. If the critical section has been entered then the cmpxchg succeeded for the uncontended case and failed for the contended case. In the success case the kernel must clear the pointer. In the failed case the kernel is not allowed to touch it. So the tests should validate both scenarios. You can differentiate between the cases via lockval =3D=3D child and lockval =3D=3D child | FUTEX_WAITERS or via a state flag. Also in the success case it must reach the success label. In the failure case it's not allowed to do so. I think you can simplify the state handling as well. if (ptrace(PTRACE_GETREGS, child, 0, ®s) !=3D 0) errx(1, "PTRACE_GETREGS"); rhead_val =3D ptrace(....); if (test_32bit) rhead_val &=3D UINT_MAX; lock_val =3D ptrace(....); /* Get the expected value to see which variant is tested */ lock_exp =3D ptrace(....); ASSERT_EQ(lock_exp & ~FUTEX_WAITERS, child); switch (state) { case STATE_WAIT: if (pc_is_within_function(regs, is_32bit)) state =3D STATE_IN_FUNCTION; break; =20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20 case STATE_IN_FUNCTION: ASSERT_NE(rhead_val, 0); ASSERT_EQ(lock_val, lock_exp); if (pc_is_within_cs(regs, is_32bit)) state =3D STATE_IN_CS; break; case STATE_IN_CS: if (lock_exp & FUTEX_WAITERS) { ASSERT_EQ(lock_val, lock_exp); ASSERT_NE(rhead_val, 0); ASSERT_NE(pc_within_success(regs, is_32bit)= , true); } else { ASSERT_EQ(lock_val, 0); ASSERT_NE(rhead_val, 0); if (pc_within_success(...)) state =3D STATE_IN_SUCCESS; else ASSERT_EQ(pc_within_cs(...), true); } break; ... } You get the idea. > + > +TEST(robust_unlock_64) > +{ > + pid_t child; > + > + if (!frtu64) > + SKIP(return, "Missing __vdso_futex_robust_list64_try_unlock() in VDSO\= n"); Which is valid when this is built as 32-bit executable, so the test should not be there at all. Thanks, tglx