Re: [PATCH v8 1/4] arm64: vdso: Prepare for robust futex unlock support

[Mark Rutland]

Hi André,

I have a few comments here; mostly minor nits.

On Fri, Aug 21, 2026 at 06:50:42PM -0300, André Almeida wrote:
> To solve the robust futex's list_pending_op clearing race condition,
> prepare for implement __vdso_futex_robust_try_unlock() for arm64 with the
> following steps:
> 
> - Create a helper function that sets the struct futex_mm_data with the
> VDSO's labels addresses. The robust futex fixup mechanism needs to
> compare the current instruction pointer to the VDSO instructions range.
> 
> - Split vdso_mremap() in vdso_mremap() and aarch32_mremap(), this allows
> the VDSO to be setup correctly regarding the instructions addresses for
> both ABIs when a mremap happens.

When I commented back on v5, I'd meant that the mremap changes should be
a separate patch. I've included a patch for that below; are you're happy
to take that as a prefix of this series?

> - Implement arch_futex_robust_unlock_get_pop() for arm64, checking for r2
> and r3 registers values for the fixup function. The role of this registers
> is explained in the commit that implement the assembly portion of the VDSO.
> 
> Signed-off-by: André Almeida <andrealmeid@xxxxxxxxxx>
> ---
> v6:
>  - Restructured this commit. Move the arch bits away, kept just the
>  generic/helper functions.
> 
> v4:
> - Guard symbols from vdso.lds.S with ifdef
> - drop update_ips() from sigpage remap function
> 
> v3:
>  - Fix adding vdso base addr twice
>  - Call vdso_futex_robust_unlock_update_ips() on remap as well
> v2:
>  - Fixed linker not finding VDSO symbols
> ---
> ---
>  arch/arm64/include/asm/futex_robust.h | 19 +++++++++++++++++++
>  arch/arm64/kernel/vdso.c              | 27 ++++++++++++++++++++++++++-
>  2 files changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/futex_robust.h b/arch/arm64/include/asm/futex_robust.h
> new file mode 100644
> index 000000000000..4ff783bb2dc3
> --- /dev/null
> +++ b/arch/arm64/include/asm/futex_robust.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_ARM64_FUTEX_ROBUST_H
> +#define _ASM_ARM64_FUTEX_ROBUST_H
> +
> +#include <asm/ptrace.h>
> +
> +static __always_inline void __user *arm64_futex_robust_unlock_get_pop(struct pt_regs *regs)
> +{
> +	/*
> +	 * w3 stores the result of the stlxr instruction. If it's zero, the then
> +	 * the ll/sc cmpxchg succeeded and the pending op pointer needs to be cleared.
> +	 */

It would be good if the comment could refer to the functions with the
critical sections, e.g.

	/*
	 * In the asm for __vdso_futex_robust_list{64,32}_try_unlock(), ...
	 */

That way it will be easier for folk to cross-reference this later.

> +	return (regs->user_regs.regs[3]) ? NULL : (void __user *) regs->user_regs.regs[2];

You can use 'regs->regs[n]' in place of 'regs->user_regs.regs[n]' here,
which will make this a bit shorter and easier to read.

I reckon this might also be clearer as:

| 	if (regs->regs[3])
| 		return NULL;
| 	
| 	return (void __user *)regs->regs[2];

Do we need a __force cast here, or is sparse happy without that?

> +}
> +
> +#define arch_futex_robust_unlock_get_pop(regs) \
> +	arm64_futex_robust_unlock_get_pop(regs)
> +
> +#endif /* _ASM_ARM64_FUTEX_ROBUST_H */
> diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
> index 592dd8668de4..3ef331b5b240 100644
> --- a/arch/arm64/kernel/vdso.c
> +++ b/arch/arm64/kernel/vdso.c
> @@ -11,6 +11,7 @@
>  #include <linux/clocksource.h>
>  #include <linux/elf.h>
>  #include <linux/err.h>
> +#include <linux/futex.h>
>  #include <linux/errno.h>
>  #include <linux/gfp.h>
>  #include <linux/kernel.h>
> @@ -57,6 +58,22 @@ static struct vdso_abi_info vdso_info[] __ro_after_init = {
>  #endif /* CONFIG_COMPAT_VDSO */
>  };
>  
> +#ifdef CONFIG_FUTEX_ROBUST_UNLOCK
> +static inline void __vdso_futex_update_ips(struct mm_struct *mm, bool is_32bit, void *startp,
> +					   void *endp)
> +{
> +	unsigned long start = (unsigned long) startp;
> +	unsigned long end = (unsigned long) endp;

Nit: there shouldn't be a space between the cast and the expression:

	unsigned long start = (unsigned long)startp;
	unsigned long end = (unsigned long)endp;

> +	struct futex_mm_data *fd = &mm->futex;
> +
> +	futex_set_vdso_cs_range(fd, is_32bit ? 1 : 0, start, end, is_32bit);

On arm64 (and every architecture other than x86, AFAICT), the
native/compat VDSOs are mutually exclusive, and a single mm can only
have one of them.

Given that, I think we can make this:

	futex_set_vdso_cs_range(fd, 0, start, end, is_32bit);

That way we'll avoid confusing/bikesheeding over 'is_32bit ? 1 : 0',
without having to add mnemnonics for the native/compat CS indices.

That said, what's the plan for 32-bit robust lists on a 64-bit host?
IIUC you wanted that for emulation, and AFAICT you have no way to call
__vdso_futex_robust_list32_try_unlock() from a native task.

> +}
> +
> +#else
> +static inline void __vdso_futex_update_ips(struct mm_struct *mm, bool is_32bit, void *startp,
> +					   void *endp)
> +#endif /* CONFIG_FUTEX_ROBUST_UNLOCK */
> +
>  static int vdso_mremap(const struct vm_special_mapping *sm,
>  		struct vm_area_struct *new_vma)
>  {
> @@ -162,6 +179,14 @@ static int aarch32_sigpage_mremap(const struct vm_special_mapping *sm,
>  	return 0;
>  }
>  
> +static int aarch32_mremap(const struct vm_special_mapping *sm,
> +		struct vm_area_struct *new_vma)
> +{
> +	current->mm->context.vdso = (void *)new_vma->vm_start;
> +
> +	return 0;
> +}
> +
>  static struct vm_special_mapping aarch32_vdso_maps[] = {
>  	[AA32_MAP_VECTORS] = {
>  		.name	= "[vectors]", /* ABI */
> @@ -174,7 +199,7 @@ static struct vm_special_mapping aarch32_vdso_maps[] = {
>  	},
>  	[AA32_MAP_VDSO] = {
>  		.name = "[vdso]",
> -		.mremap = vdso_mremap,
> +		.mremap = aarch32_mremap,
>  	},
>  };

As above, I'd prefer if the mremap changes were a separate patch, e.g.
as below.

Mark.

---->8----