[PATCH v2] btrfs: flush the fixup workers during close_ctree

Reintroducing the COW fixup worker brought back the unmount race fixed
by commit 41fd1e94066a ("btrfs: wait for fixup workers before stopping
cleaner kthread during umount") without bringing back the fix.

A fixup work item queued by the final writeback pass can still be in flight
when close_ctree() stops the cleaner kthread and frees the fs roots.
While destroy_workqueue() drains the queue, that happens after the
cleaner thread was freed, so btrfs_add_delayed_iput() called from the
fixup worker is no longer safe (not to mention that we are already in
BTRFS_FS_STATE_NO_DELAYED_IPUT when it runs).

Therefore we need to bring back explicitly flushing the fixup workqueue
as in Filipe's original fix. The first flush will catch all the fixup
writeback queued during the final sync before umount, but some of that
might hit memory allocation errors and stay fixup in the blocks/folio,
leading any subsequent writeback triggered *inside* umount (e.g. reclaim
workers shutting down) to hit it and queue again. To fix that, and the
possibility of any really long-lived pinned folios getting marked, deny
queueing new fixup during umount. That allows us to flush twice (once
before doing a real writeback pass to get the actual data, second time
to clean up any rather unlikely stragglers right before declaring
BTRFS_FS_STATE_NO_DELAYED_IPUT) and be certain nothing got re-queued.

Reproduced by injecting a one-shot 30s sleep at the head of
btrfs_writepage_fixup_worker() on a KASAN kernel, running the normal
reproducing read dio workload before unmount and then observing:

  BUG: KASAN: slab-use-after-free in _raw_spin_lock_irqsave+0x35/0x50
  Read of size 1 at addr ffff88810b4b08f8 by task kworker/u32:5/219
  Workqueue: btrfs-fixup btrfs_writepage_fixup_worker [btrfs]
  Call Trace:
   _raw_spin_lock_irqsave+0x35/0x50
   try_to_wake_up+0xc0/0x18c0
   btrfs_writepage_fixup_worker+0x7f3/0xf20 [btrfs]
  ...

Fixes: 4be9c7da6860 ("btrfs: trigger cow fixup via dirty_folio()")
Assisted-by: LLM (reproduction, analysis)
Signed-off-by: Boris Burkov <[email protected]>
---
Changelog:
v2:
- clear fixup on skip, following the model of errors in the worker

 fs/btrfs/disk-io.c | 21 +++++++++++++++++++++
 fs/btrfs/inode.c   | 17 +++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 7a2f7006085d..cc4dcd10631a 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4377,6 +4377,18 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
 	/* clear out the rbtree of defraggable inodes */
 	btrfs_cleanup_defrag_inodes(fs_info);
 
+	/*
+	 * Before the unmount, we sync down all the writeback which can
+	 * generate fixup work. We are about to run delalloc for autodefrag so
+	 * piggy back on that by also flushing the fixup work which can also
+	 * generate delalloc we would like to get run.
+	 *
+	 * After this, it is still possible that some thread doing writeback is
+	 * in btrfs_queue_writepage_fixup() and might finish queueing some final
+	 * work, racing the btrfs_fs_closing() check there.
+	 */
+	flush_workqueue(fs_info->fixup_workers);
+
 	/*
 	 * After we entered close_ctree() autodefrag could be running and before
 	 * we parked the cleaner kthread, it dirtied folios of some inode.
@@ -4469,6 +4481,15 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
 	cancel_work_sync(&fs_info->preempt_reclaim_work);
 	cancel_work_sync(&fs_info->em_shrinker_work);
 
+	/*
+	 * Reclaim workers can run writeback which can queue fixup.
+	 * After the above cancel_work_sync() calls, any such queueing attempts are
+	 * guaranteed to see btrfs_fs_closing(), so at this point we can genuinely fully
+	 * flush the fixup workqueue. This relies on the belief that *now* no thread can
+	 * still be sitting in btrfs_queue_writepage_fixup().
+	 */
+	flush_workqueue(fs_info->fixup_workers);
+
 	/*
 	 * Run delayed iputs again because an async reclaim worker may have
 	 * added new ones if it was flushing delalloc:
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 78143e241ca4..04efe183d57b 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2964,6 +2964,23 @@ void btrfs_queue_writepage_fixup(struct btrfs_inode *inode, struct folio *folio)
 	struct btrfs_fs_info *fs_info = inode->root->fs_info;
 	struct btrfs_writepage_fixup *fixup;
 
+	/*
+	 * Disallow queueing more fixup during unmount to break the cycle
+	 * of writeback queuing fixup queuing writeback etc.
+	 *
+	 * If it actually hit, then something which was fixup wasn't written
+	 * which we should warn about.
+	 */
+	if (btrfs_fs_closing(fs_info)) {
+		btrfs_warn_rl(fs_info,
+	"dropping unqueued fixup blocks at unmount. root %lld ino %llu folio %llu",
+			      btrfs_root_id(inode->root), btrfs_ino(inode),
+			      folio_pos(folio));
+		btrfs_folio_clear_fixup_dirty(fs_info, folio,
+					      folio_pos(folio), folio_size(folio));
+		return;
+	}
+
 	fixup = kzalloc_obj(*fixup, GFP_NOFS);
 	if (!fixup)
 		return;
-- 
2.55.0