[PATCH RFC -rcu/dev] hazptr: Record per-CPU slot owner after claiming the slot

[Andrea Parri <parri.andrea@gmail.com>]

An interrupt that uses a hazard pointer while the interrupted task is
in the acquire slow path can leave its own context recorded as the
owner of the per-CPU slot the task then publishes into.  The next
context switch promotes the task's pointer with the interrupt's
context.

hazptr_get_free_percpu_slot() records the owner when it picks an empty
slot, but only Store B in __hazptr_try_acquire() claims the slot.  An
interrupt in between picks the same slot, as the lowest free one:

  Task on CPU 0                        IRQ on CPU 0
  -------------                        ------------
  hazptr_acquire(ctx)
    fast path fails
    __hazptr_acquire(ctx)
      picks item k (empty)
      item k->ctx.ctx = ctx
                                       hazptr_acquire(irq_ctx)
                                         claims item k
                                         item k->ctx.ctx = irq_ctx
                                       hazptr_release(irq_ctx)
                                         clears slot k
      __hazptr_try_acquire()
        Store B into slot k
      (item k still names irq_ctx)
  sleeps holding the pointer
  hazptr_note_context_switch()
    promotes item k with irq_ctx

The task gets there with k > 0 when it already holds an outer hazard
pointer in slot 0; no other task can, as its slot is promoted when it
is switched out.

The task gets there with k == 0 when the fast path leaves slot 0
empty: either its ptr_eq() check fails because an updater changed the
pointer, or the pointer is NULL and an updater publishes a non-NULL one
before the slow path loads it.

The promotion moves the task's pointer into irq_ctx's backup slot,
which nothing unchains, so hazptr_synchronize() on that pointer never
returns.  The task's ctx->slot still names slot k, so its
hazptr_release() clears whatever another reader on that CPU has since
published there.

If the interrupt detaches instead, as hazptr_torture_acquire() does,
the promotion chains irq_ctx's backup slot a second time while it is
still linked.  The overflow list then loops on that node, or follows
LIST_POISON1 once hazptr_unchain_backup_slot() removes it.

Record the owner after __hazptr_try_acquire() succeeds, as the fast
path in hazptr_acquire() does.  This should be safe because an
interrupt that used the slot has released or detached it by the time
the task resumes, and only hazptr_note_context_switch() reads the
owner, which cannot run before the task enables preemption.

Fixes: 50b568234163 ("hazptr: Implement Hazard Pointers")
Assisted-by: LLM
Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
---

Notes:
    With this patch, hazptrtorture's NOPREEMPT scenario passes a 30-minute
    run on x86_64 with 8 CPUs.  PREEMPT runs clean for 30 minutes but can
    hang at shutdown, in hazptrtorture's release of deferred hazard
    pointers; that hang does not involve this patch, and is addressed by
    "hazptrtorture: Release all deferred hazard pointers before stopping",
    posted separately.
    
    With that fix also applied, 30-minute runs pass for PREEMPT, for
    NOPREEMPT with torture_type=hazptr, for PREEMPT with
    torture_type=hazptr-stack, and for PREEMPT with CONFIG_HAZPTR_DEBUG=n.
    
    PREEMPT and NOPREEMPT also pass without this patch: their readers hold
    one hazard pointer each, so they reach the slow path only when the
    ptr_eq() check fails or the pointer is NULL, which happens only before
    the writer publishes its first element and after it stops.
    
    A KUnit reproducer, with a test-only hook in __hazptr_try_acquire()
    between the free-slot check and Store B, fails on 940793b926aa and
    passes with this patch.  I can post it if useful.

 kernel/hazptr.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/kernel/hazptr.c b/kernel/hazptr.c
index 3ca73b56a5c25..b15b84ebd4ceb 100644
--- a/kernel/hazptr.c
+++ b/kernel/hazptr.c
@@ -54,19 +54,16 @@ unsigned int flip_list_phase(unsigned int phase)
 }
 
 static
-struct hazptr_slot *hazptr_get_free_percpu_slot(struct hazptr_ctx *ctx)
+struct hazptr_slot_item *hazptr_get_free_percpu_slot_item(void)
 {
 	struct hazptr_percpu_slots *percpu_slots = this_cpu_ptr(&hazptr_percpu_slots);
 	unsigned int idx;
 
 	for (idx = 0; idx < NR_HAZPTR_PERCPU_SLOTS; idx++) {
 		struct hazptr_slot_item *item = &percpu_slots->items[idx];
-		struct hazptr_slot *slot = &item->slot;
 
-		if (!slot->addr) {
-			item->ctx.ctx = ctx;
-			return slot;
-		}
+		if (!item->slot.addr)
+			return item;
 	}
 	/* All slots are in use. */
 	return NULL;
@@ -78,6 +75,7 @@ struct hazptr_slot *hazptr_get_free_percpu_slot(struct hazptr_ctx *ctx)
  */
 void *__hazptr_acquire(struct hazptr_ctx *ctx, void * const *addr_p)
 {
+	struct hazptr_slot_item *item;
 	struct hazptr_slot *slot;
 	void *addr;
 
@@ -85,10 +83,17 @@ void *__hazptr_acquire(struct hazptr_ctx *ctx, void * const *addr_p)
 	 * In case we are called due to nested use of hazard pointers,
 	 * try a slot protection with per-CPU slots.
 	 */
-	slot = hazptr_get_free_percpu_slot(ctx);
-	if (likely(slot)) {
+	item = hazptr_get_free_percpu_slot_item();
+	if (likely(item)) {
+		slot = &item->slot;
 		addr = __hazptr_try_acquire(ctx, addr_p, slot);
 		if (addr) {
+			/*
+			 * Record the owner only once Store B has claimed the
+			 * slot, as hazptr_acquire() does: an interrupt may
+			 * have used this slot since it was chosen.
+			 */
+			item->ctx.ctx = ctx;
 			ctx->slot = slot;
 			return addr;
 		}

-- 
2.53.0