[PATCH RFC] __futex_pivot_hash() race

[PATCH RFC] __futex_pivot_hash() race

[Chris Mason]

This is really just AI questioning how we synchronize mmph->batches and
mmph->hash.  It looks real to me, and the repro with printks does fire,
so hopefully I've got things right.

The patch and description were also AI, meaning this is more bug
report in patch form.  Testing was only done against the one repro.

(I may or may not have successfully switched over my git config to
mason@kernel.org, lets see...)

[PATCH] futex: sample poll cookie after publishing new hash

[Chris Mason]

futex_ref_drop() may only skip its grace period when one has already
elapsed since the current private hash was published:

    kernel/futex/core.c:futex_ref_drop
        if (poll_state_synchronize_rcu(mm->futex.phash.batches)) {
                /*
                 * There was a grace-period, we can begin now.
                 */
                __futex_ref_atomic_begin(fph);
                return;
        }

The cookie it polls is sampled one statement before that publication:

    kernel/futex/core.c:__futex_pivot_hash
        new->state = FR_PERCPU;
        scoped_guard(rcu) {
                mmph->batches = get_state_synchronize_rcu();
                rcu_assign_pointer(mmph->hash, new);
        }
        kvfree_rcu(fph, rcu);

get_state_synchronize_rcu() anchors its guarantee at the snapshot, so
the cookie is cleared by the first grace period that starts from there
on, including one starting between the two stores which never waited
for a reader that loaded the old hash after it began.

    CPU 0 (resize)                    CPU 1 (futex_hash)
    ==============                    ==================
    __futex_pivot_hash()
      batches = get_state_...()
                                      grace period starts
                                      guard(rcu)
                                      fph = old hash
      rcu_assign_pointer(hash, new)
      kvfree_rcu(old hash)
    futex_hash_allocate()
      futex_ref_drop(new hash)
        poll_state_...() -> true
        __futex_ref_atomic_begin()
          atomic = LONG_MAX
                                      futex_ref_get(old hash) -> true
                                      spin_lock(&fph->queues[i].lock)

The reference count lives in the mm and has just been biased for the
new generation, so the stalled reader pins and then locks the retired
hash that is already queued for free, and its later put is charged
against the live generation.

Fix by sampling the cookie after rcu_assign_pointer() publishes the
new hash. Drop the surrounding scoped_guard(rcu) while at it: it only
delayed completion of the prematurely anchored grace-period and serves
no purpose once the cookie is sampled after publication. The writer
side is serialized by mm->futex.phash.lock and neither
rcu_assign_pointer() nor kvfree_rcu() requires a read-side section.

Fixes: 56180dd20c19 ("futex: Use RCU-based per-CPU reference counting instead of rcuref_t")
Assisted-by: kres:claude-opus-5
Signed-off-by: Chris Mason <mason@kernel.org>
---
 kernel/futex/core.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/kernel/futex/core.c b/kernel/futex/core.c
index 128c5752f225..05619eb8329c 100644
--- a/kernel/futex/core.c
+++ b/kernel/futex/core.c
@@ -209,10 +209,14 @@ static bool __futex_pivot_hash(struct mm_struct *mm, struct futex_private_hash *
 		futex_rehash_private(fph, new);
 	}
 	new->state = FR_PERCPU;
-	scoped_guard(rcu) {
-		mmph->batches = get_state_synchronize_rcu();
-		rcu_assign_pointer(mmph->hash, new);
-	}
+	rcu_assign_pointer(mmph->hash, new);
+	/*
+	 * Pairs with futex_ref_drop(): ->batches must be sampled at or after
+	 * the rcu_assign_pointer() above, so any grace-period satisfying
+	 * poll_state_synchronize_rcu() provably started once no reader could
+	 * still load the retired fph. Both stores are done under mmph->lock.
+	 */
+	mmph->batches = get_state_synchronize_rcu();
 	kvfree_rcu(fph, rcu);
 	return true;
 }
-- 
2.53.0-Meta