btrfs: stripe_alloc: hold a reference on the rbio being parked

rbio_try_park() publishes the rbio on stripe_hash_table->parked and then
keeps reading it: it drops parked_lock, queues the scan timer, and
rechecks rbio_is_full() to reclaim a park that a concurrent merge has
just filled.  Nothing keeps the rbio alive across that.

A parked rbio has exactly two references, the initial one and the one
the stripe hash list took in lock_stripe_add(), and its completion
consumes both.  So the moment parked_lock is dropped, a flusher
(btrfs_flush_parked_rbios(), unpark_ready_rbio(), or the park timer) can
take the rbio, start its RMW, and have it complete through
rbio_orig_end_io() -> unlock_stripe() -> free_raid_bio() -- while the
parking thread is still inside rbio_try_park(), about to dereference it.

lockdep sees it first, because rbio_is_full() takes rbio->bio_list_lock
and a freed rbio's lockdep key is gone:

  INFO: trying to register non-static key.
  turning off the locking correctness validator.
  Workqueue: btrfs-rmw rmw_rbio_work
   register_lock_class+0x565/0x570
   __lock_acquire+0x3a8/0x23d0
   lock_acquire+0xe5/0x330
   _raw_spin_lock+0x3b/0x90
   rmw_rbio_work+0x256/0x400

and 62 ms later the same worker falls over the list_head next to it:

  list_del corruption, ffff8881810a56e0->next is NULL
  WARNING: CPU: 3 PID: 715967 at lib/list_debug.c:52
           __list_del_entry_valid_or_report+0x7b/0x150
  Workqueue: btrfs-rmw rmw_rbio_work

That list_head is parked_node, at offset 0xe0 of struct btrfs_raid_bio.
The task then took a NULL dereference holding parked_lock, and every
other CPU piled into __pv_queued_spin_lock_slowpath() behind it: flushes
from transaction commit and the ordered-extent waiters, the park timeout
work, and other rmw_rbio_work()s.  The box wedged with all CPUs spinning.

Most interleavings survive by luck, which is why this is rare: the
recheck tests RBIO_PARKED_BIT, and a legitimate unpark clears that bit
under parked_lock, so the stale thread usually just sees it clear and
returns.  It stops being luck once the freed object has been reused.

Take a reference before publishing and drop it after the recheck.  It
has to belong to the parking thread rather than to the parked list: a
list-owned reference is inherited and dropped by whoever unparks the
rbio, and that is precisely the thread racing us, so it protects
everything except the window it was meant to protect.  This one cannot
be consumed by anyone else.

Dropping it is never the last put -- a parked rbio still owns the stripe
lock, and only the RMW a flusher starts after taking it off the list
completes it -- so the free still happens where it always did.

Assisted-by: Claude:claude-fable-5-1

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 60ad082942785e3e3f825893560cd2dc400081bf..80d307202e574d04bbf458f465a7ffef5b004fc5 100644 (file)
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -894,6 +894,21 @@ static bool rbio_try_park(struct btrfs_raid_bio *rbio)
        rbio->park_deadline = jiffies + msecs_to_jiffies(timeout_ms);
        rbio->park_stuck_deadline = jiffies +
                msecs_to_jiffies(timeout_ms * 10);
+       /*
+        * Take a reference of our own before publishing.  The moment
+        * parked_lock is dropped this rbio is visible to every flusher, and a
+        * flusher that claims it starts the RMW, which runs to completion and
+        * frees the rbio -- while we are still in this function, about to read
+        * it again in the fullness recheck below.  Its other two references,
+        * the initial one and the stripe hash list's, are both consumed by
+        * that completion, so neither protects us.
+        *
+        * It has to be OUR reference, not one owned by the parked list: an
+        * unparker would inherit a list reference and drop it, and the
+        * unparker is exactly the thread racing us here.  Nobody can consume
+        * this one but us.
+        */
+       refcount_inc(&rbio->refs);
        list_add_tail(&rbio->parked_node, &table->parked);
        atomic_inc(&fs_info->stripe_parked_now);
        spin_unlock(&table->parked_lock);
@@ -911,10 +926,18 @@ static bool rbio_try_park(struct btrfs_raid_bio *rbio)
                if (test_bit(RBIO_PARKED_BIT, &rbio->flags)) {
                        rbio_unpark_locked(rbio);
                        spin_unlock(&table->parked_lock);
+                       free_raid_bio(rbio);
                        return false;
                }
                spin_unlock(&table->parked_lock);
        }
+       /*
+        * Done reading it.  Dropping our reference here cannot be the last
+        * one: a parked rbio still owns the stripe lock, and the only thing
+        * that completes it is the RMW a flusher starts after taking it off
+        * the list.
+        */
+       free_raid_bio(rbio);
        return true;
 }