Re: [RFC PATCH v4] futex: Introduce __vdso_robust_futex_unlock and __vdso_robust_pi_futex_try_unlock

[Thomas Weißschuh]

Hi Mathieu,

some small remarks around the vDSO code.

On Fri, Mar 13, 2026 at 09:39:03AM -0400, Mathieu Desnoyers wrote:

(...)

> The approach taken to fix this is to introduce two vDSO and extend the
> x86 vDSO exception table to track the relevant ip ranges: one for non-PI
> robust futexes, and one for PI robust futexes.

One of the central points behind the vDSO so far was that it is only a
performance optimization. It is never required for correctness.
What are applications supposed to do when the vDSO is disabled?

(...)

> diff --git a/arch/x86/entry/vdso/common/vfutex.c b/arch/x86/entry/vdso/common/vfutex.c
> new file mode 100644
> index 000000000000..336095b04952
> --- /dev/null
> +++ b/arch/x86/entry/vdso/common/vfutex.c
> @@ -0,0 +1,90 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2026 Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
> + */
> +#include <linux/types.h>
> +#include <linux/futex.h>

This should be uapi/linux/futex.h. Headers from the linux/ namespace should
not be used in vDSO code. The definitions from them may end up being wrong
in the compat vDSO. Either use uapi/ or vdso/ headers. (linux/types.h is a bit
of an exception for historic reasons, it could be replaced by uapi/linux/types.h)

> +#include <vdso/futex.h>
> +#include "extable.h"

> +
> +#ifdef CONFIG_X86_64

This only works because of the ugly hacks in fake_32bit_build.h.
Testing for '#ifdef __x86_64__' is simpler and nicer to read.

> +# define ASM_PTR_BIT_SET	"btsq "
> +# define ASM_PTR_SET		"movq "
> +#else
> +# define ASM_PTR_BIT_SET	"btsl "
> +# define ASM_PTR_SET		"movl "
> +#endif
> +
> +u32 __vdso_robust_futex_unlock(u32 *uaddr, struct robust_list_head *robust_list_head)
> +{
> +	u32 val = 0;
> +
> +	/*
> +	 * Within the ip range identified by the futex exception table,
> +	 * the register "eax" contains the value loaded by xchg. This is
> +	 * expected by futex_vdso_exception() to check whether waiters
> +	 * need to be woken up. This register state is transferred to
> +	 * bit 1 (NEED_ACTION) of *op_pending_addr before the ip range
> +	 * ends.
> +	 */
> +	asm volatile (
> +		_ASM_VDSO_EXTABLE_FUTEX_HANDLE(1f, 3f)
> +		/* Exchange uaddr (store-release). */
> +		"xchg %[uaddr], %[val]\n\t"
> +		"1:\n\t"
> +		/* Test if FUTEX_WAITERS (0x80000000) is set. */
> +		"test %[val], %[val]\n\t"
> +		"js 2f\n\t"
> +		/* Clear *op_pending_addr if there are no waiters. */
> +		ASM_PTR_SET "$0, %[op_pending_addr]\n\t"
> +		"jmp 3f\n\t"
> +		"2:\n\t"
> +		/* Set bit 1 (NEED_ACTION) in *op_pending_addr. */
> +		ASM_PTR_BIT_SET "$1, %[op_pending_addr]\n\t"
> +		"3:\n\t"
> +		: [val] "+a" (val),
> +		  [uaddr] "+m" (*uaddr)
> +		: [op_pending_addr] "m" (robust_list_head->list_op_pending)
> +		: "memory"
> +	);
> +	return val;
> +}
> +
> +u32 robust_futex_unlock(u32 *, struct robust_list_head *)
> +	__attribute__((weak, alias("__vdso_robust_futex_unlock")));

__weak and __alias() as per checkpatch.pl.

The entries in the linkerscripts are missing.

(...)

> --- /dev/null
> +++ b/include/vdso/futex.h
> @@ -0,0 +1,72 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2026 Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
> + */
> +
> +#ifndef _VDSO_FUTEX_H
> +#define _VDSO_FUTEX_H
> +
> +#include <linux/types.h>
> +#include <linux/futex.h>

Same remarks about the linux/ namespace as before.

> +/**
> + * __vdso_robust_futex_unlock - Architecture-specific vDSO implementation of robust futex unlock.
> + * @uaddr:		Lock address (points to a 32-bit unsigned integer type).
> + * @robust_list_head:	The thread-specific robust list that has been registered with set_robust_list.
> + *
> + * This vDSO unlocks the robust futex by exchanging the content of
> + * *@uaddr with 0 with a store-release semantic. If the futex has
> + * waiters, it sets bit 1 of *@robust_list_head->list_op_pending, else
> + * it clears *@robust_list_head->list_op_pending. Those operations are
> + * within a code region known by the kernel, making them safe with
> + * respect to asynchronous program termination either from thread
> + * context or from a nested signal handler.
> + *
> + * Returns:	The old value present at *@uaddr.
> + *
> + * Expected use of this vDSO:
> + *
> + * robust_list_head is the thread-specific robust list that has been
> + * registered with set_robust_list.
> + *
> + * if ((__vdso_robust_futex_unlock((u32 *) &mutex->__data.__lock, robust_list_head)
> + *     & FUTEX_WAITERS) != 0)
> + *         futex_wake((u32 *) &mutex->__data.__lock, 1, private);
> + * WRITE_ONCE(robust_list_head->list_op_pending, 0);
> + */
> +extern u32 __vdso_robust_futex_unlock(u32 *uaddr, struct robust_list_head *robust_list_head);

Drop the extern.

(...)

> +#endif /* _VDSO_FUTEX_H */

(...)