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

[Peter Zijlstra]

On Tue, Oct 27, 2026 at 12:37:13AM +0100, David Hildenbrand (Arm) wrote:
> 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@xxxxxxxxxx>
> >>>
> >>> 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.

This looks quite terrible, doubly so because I have no idea what its
trying to fix. Please try and justify this insanity.

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