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

[Kiryl Shutsemau]

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 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);
-- 
  Kiryl Shutsemau / Kirill A. Shutemov