Re: [PATCHv7 13/18] mm: Drop fake head checks

["David Hildenbrand (Arm)" <david@kernel.org>]

On 10/27/26 00:25, Kiryl Shutsemau wrote:
> On Mon, Oct 26, 2026 at 06:45:48PM +0100, David Hildenbrand (Arm) wrote:
>> On 2/27/26 20:42, Kiryl Shutsemau (Meta) wrote:
>>> From: Kiryl Shutsemau <kas@kernel.org>
>>>
>>> With fake head pages eliminated in the previous commit, remove the
>>> supporting infrastructure:
>>>
>>>   - page_fixed_fake_head(): no longer needed to detect fake heads;
>>>   - page_is_fake_head(): no longer needed;
>>>   - page_count_writable(): no longer needed for RCU protection;
>>>   - RCU read_lock in page_ref_add_unless(): no longer needed;
>>
>> I have an AI report that removing RCU read-lock was wrong.
> 
> Ughh. Yes, it is a real bug.
> 
> My reasoning in the commit message was wrong. Nothing stops a
> speculative get from writing to one after the remap. On x86 even a
> failing cmpxchg faults there.
> 
> The fix I have in mind is to let the atomic fault.
> 
> If the update faults, the struct page is a read-only HVO tail with a
> zero refcount, so the get just fails. An exception table entry on the
> cmpxchg handles it. No RCU on either side, and the fast path stays the
> same lock cmpxchg loop.
> 
> Only x86, riscv and loongarch have HVO. Below is x86 only, to show the
> idea; riscv and loongarch need the same primitive in their LL/SC
> fetch_add_unless().
> 
> Lightly tested on x86 by hitting a real HVO tail page.
> 
> Does it look sane?

I'm sure x86 folks will have an opinion :) . Let me CC them whether something
like that would be acceptable.

> 
> I can prepare a proper patchset if looks reasonable.
> 
> diff --git a/arch/x86/include/asm/atomic.h b/arch/x86/include/asm/atomic.h
> index 75743f1dfd4e..5252b1382ed0 100644
> --- a/arch/x86/include/asm/atomic.h
> +++ b/arch/x86/include/asm/atomic.h
> @@ -108,6 +108,25 @@ static __always_inline bool arch_atomic_try_cmpxchg(atomic_t *v, int *old, int n
>  }
>  #define arch_atomic_try_cmpxchg arch_atomic_try_cmpxchg
>  
> +/*
> + * arch_atomic_fetch_add_unless_nofault() is arch_atomic_fetch_add_unless(),
> + * except that a fault on the access jumps to @label instead of oopsing.  It
> + * works in any context, including with IRQs disabled.
> + */
> +#define arch_atomic_fetch_add_unless_nofault(_v, _a, _u, label)	\
> +({									\
> +	atomic_t *__v = (_v);						\
> +	int __a = (_a), __u = (_u);					\
> +	int __c = arch_atomic_read(__v);				\
> +									\
> +	do {								\
> +		if (unlikely(__c == __u))				\
> +			break;						\
> +	} while (!__try_cmpxchg_nofault(&__v->counter, &__c,		\
> +					__c + __a, label));		\
> +	__c;								\
> +})
> +
>  static __always_inline int arch_atomic_xchg(atomic_t *v, int new)
>  {
>  	return arch_xchg(&v->counter, new);
> diff --git a/arch/x86/include/asm/cmpxchg.h b/arch/x86/include/asm/cmpxchg.h
> index a88b06f1c35e..685aab3edd3f 100644
> --- a/arch/x86/include/asm/cmpxchg.h
> +++ b/arch/x86/include/asm/cmpxchg.h
> @@ -232,6 +232,63 @@ extern void __add_wrong_size(void)
>  #define arch_try_cmpxchg_local(ptr, pold, new)				\
>  	__try_cmpxchg_local((ptr), (pold), (new), sizeof(*(ptr)))
>  
> +/*
> + * __try_cmpxchg_nofault() is arch_try_cmpxchg() on a 4-byte kernel variable,
> + * except that a fault on the access jumps to @label instead of oopsing.
> + */
> +#ifdef CONFIG_CC_HAS_ASM_GOTO_TIED_OUTPUT
> +#define __try_cmpxchg_nofault(_ptr, _pold, _new, label)		\
> +({									\
> +	bool success;							\
> +	__typeof__(_ptr) _old = (__typeof__(_ptr))(_pold);		\
> +	__typeof__(*(_ptr)) __old = *_old;				\
> +	__typeof__(*(_ptr)) __new = (_new);				\
> +									\
> +	if (sizeof(*(_ptr)) != 4)					\
> +		__cmpxchg_wrong_size();					\
> +	asm_goto_output("\n"						\
> +			"1: " LOCK_PREFIX "cmpxchgl %[new], %[ptr]\n"	\
> +			_ASM_EXTABLE(1b, %l[label])			\
> +			: "=@ccz" (success),				\
> +			  [ptr] "+m" (*(_ptr)),				\
> +			  [old] "+a" (__old)				\
> +			: [new] "r" (__new)				\
> +			: "memory"					\
> +			: label);					\
> +	if (unlikely(!success))						\
> +		*_old = __old;						\
> +	likely(success);						\
> +})
> +#else
> +#define __try_cmpxchg_nofault(_ptr, _pold, _new, label)		\
> +({									\
> +	int __err = 0;							\
> +	bool success;							\
> +	__typeof__(_ptr) _old = (__typeof__(_ptr))(_pold);		\
> +	__typeof__(*(_ptr)) __old = *_old;				\
> +	__typeof__(*(_ptr)) __new = (_new);				\
> +									\
> +	if (sizeof(*(_ptr)) != 4)					\
> +		__cmpxchg_wrong_size();					\
> +	asm volatile("\n"						\
> +		     "1: " LOCK_PREFIX "cmpxchgl %[new], %[ptr]\n"	\
> +		     "2:\n"						\
> +		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG,	\
> +					   %[errout])			\
> +		     : "=@ccz" (success),				\
> +		       [errout] "+r" (__err),				\
> +		       [ptr] "+m" (*(_ptr)),				\
> +		       [old] "+a" (__old)				\
> +		     : [new] "r" (__new)				\
> +		     : "memory");					\
> +	if (unlikely(__err))						\
> +		goto label;						\
> +	if (unlikely(!success))						\
> +		*_old = __old;						\
> +	likely(success);						\
> +})
> +#endif
> +
>  /*
>   * xadd() adds "inc" to "*ptr" and atomically returns the previous
>   * value of "*ptr".
> diff --git a/include/linux/page_ref.h b/include/linux/page_ref.h
> index 9f5c75d06f76..82a2fc445ce6 100644
> --- a/include/linux/page_ref.h
> +++ b/include/linux/page_ref.h
> @@ -246,9 +246,31 @@ static inline int folio_ref_dec_return(struct folio *folio)
>  	return page_ref_dec_return(&folio->page);
>  }
>  
> +#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
> +/*
> + * HVO maps the tail struct pages of a hugetlb folio read-only.  A speculative
> + * caller can read a non-zero refcount, then the page gets freed, becomes a
> + * tail page of a hugetlb folio and is remapped before the update.  The
> + * atomic then faults.  A read-only struct page is a hugetlb tail page with a
> + * zero refcount, so the fault means no reference can be taken.
> + */
> +static __always_inline bool __page_ref_add_unless_zero(atomic_t *v, int nr)
> +{
> +	instrument_atomic_read_write(v, sizeof(*v));
> +	return arch_atomic_fetch_add_unless_nofault(v, nr, 0, fault) != 0;
> +fault:
> +	return false;
> +}
> +#else
> +static __always_inline bool __page_ref_add_unless_zero(atomic_t *v, int nr)
> +{
> +	return atomic_add_unless(v, nr, 0);
> +}
> +#endif
> +
>  static inline bool page_ref_add_unless_zero(struct page *page, int nr)
>  {
> -	bool ret = atomic_add_unless(&page->_refcount, nr, 0);
> +	bool ret = __page_ref_add_unless_zero(&page->_refcount, nr);
>  
>  	if (page_ref_tracepoint_active(page_ref_mod_unless))
>  		__page_ref_mod_unless(page, nr, ret);


-- 
Cheers,

David