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

["André Almeida" <andrealmeid@igalia.com>]

Em 16/09/2026 11:24, Mark Rutland escreveu:
> Hi André,
> 
> I have a few comments here; mostly minor nits.
> 

Thanks for the review!

> 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?
> 

Sure thing, thanks for the patch :)

>> - 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@igalia.com>
>> ---
>> 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?

I've just re-compiled those patches with C=1, and didn't find any 
warning for this.

> 
>> +}
>> +
>> +#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.

Perfect, I will simplify that then.

> 
> 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.

Yes, the plan is to be able to use the 32-bit robust lists on a 64-bit 
hosts, with a new interface (set/get_robust_list2[1]). That means that a 
64-bit kernel will be able to process an user robust list made of 32-bit 
pointers. So the emulator will call set_robust_list2() one time for it's 
own list, and another time for the emulated app list.

However, what we have agreed for the moment is that, while there's no 
support for that, we will not expose the 32-bit functions yet [2]:

"...that requires also the ability to register a 32-bit robust list for 
a 64-bit process, which is not supported right now. So no, we are not 
exposing something half functional..."

So yes, in the near future I will need to expose 
__vdso_futex_robust_list32_try_unlock() to 64-bit apps as well, to be 
able to do this handling with 32-bit pointers, but we are not doing it 
right now.

[1] 
https://lore.kernel.org/lkml/20251122-tonyk-robust_futex-v6-0-05fea005a0fd@igalia.com/

[2] https://lore.kernel.org/lkml/878q9vwote.ffs@tglx/