mmb_sync: b_mmb NULL window races mmb_mark_buffer_dirty

mmb_sync: b_mmb NULL window races mmb_mark_buffer_dirty

[Aurelien DESBRIERES]

Hi Jan,

While measuring beamfs on vfs.all (2c3ba1f1cb41, on v7.3-rc7) we hit a
list corruption in fs/buffer.c that I think is a window in mmb_sync()
rather than anything specific to our filesystem, so here it is with
the trace. beamfs was attaching buffers that several inodes share
(bitmap blocks, parity regions) to the allocating inode's
i_metadata_bhs, so the same buffer_head moved from one inode's list to
another's. We have since given those buffers a list of their own and
the problem is gone on our side; the window remains.

The window

mmb_sync() does, under mmb_A->lock:

	__remove_assoc_queue(mmb, bh);	/* list_del_init, b_mmb = NULL */
	smp_mb();
	if (buffer_dirty(bh) || buffer_locked(bh)) {
		list_add(&bh->b_assoc_buffers, &tmp);
		bh->b_mmb = mmb;

Between the NULL store and the restore, mmb_mark_buffer_dirty(bh, mmb_B)
on another CPU sees !bh->b_mmb, takes mmb_B->lock (a different lock),
its cmpxchg(&bh->b_mmb, NULL, mmb_B) succeeds, and list_move_tail()
writes bh->b_assoc_buffers while CPU A's list_add() does the same.
The second loop of mmb_sync() has the same shape (remove, then
list_add back to mmb->list when still dirty).

The old mark_buffer_dirty_inode() took the bdev mapping's
i_private_lock, shared by every inode of the device, so the two
writers could not overlap. A per-mmb lock protects a buffer that
belongs to one mmb for life; it does not protect the moment a buffer
changes hands. The comment above the cmpxchg says it is there for a
corrupted filesystem with multiply claimed blocks, and the kernel-doc
says the buffer is added "if it is not already on a list": that is
exactly the case the window lets through, so a corrupted ext2, fat,
minix or udf volume in which two inodes claim one indirect block
should be able to reach it as well, given an fsync on one inode while
the other dirties the buffer.

What it does when it hits

With CONFIG_DEBUG_LIST, __list_del_entry_valid_or_report() refuses the
del, list_del_init() does nothing, and the first loop of mmb_sync()
is `while (!list_empty(&mmb->list))` on a list whose head never
leaves: the CPU spins holding mmb->lock. We lost the node to it
(soft lockup from 48 s up, then kmemleak and the OOM killer).

  list_del corruption. next->prev should be ffffa26a6e8d4820, but was ffffa26a63804190. (next=ffffa26a432e1238)
  WARNING: /lib/list_debug.c:65 at __list_del_entry_valid_or_report+0xf6/0x110, CPU#0: dbench/16440
  CPU: 0 UID: 0 PID: 16440 Comm: dbench Not tainted 7.3.0-rc7 #1 PREEMPT(lazy)
  Call Trace:
   __remove_assoc_queue+0x20/0x80
   mmb_sync+0xba/0x300
   beamfs_write_inode+0x45/0x190
   __writeback_single_inode+0x480/0x630
   writeback_single_inode+0x11d/0x4d0
   sync_inode_metadata+0x46/0x70
   simple_fsync_noflush+0x55/0x90
   simple_fsync+0x16/0x40
   do_fsync+0x50/0xc0
   __x64_sys_fsync+0x13/0x20
  [...]
  rcu: INFO: rcu_preempt self-detected stall on CPU
  RIP: 0010:mmb_sync+0xc0/0x300
  [...]
  watchdog: BUG: soft lockup - CPU#0 stuck for 48s! [dbench:16440]
  watchdog: BUG: soft lockup - CPU#0 stuck for 413s! [dbench:16440]

xfstests generic/241 (dbench) on a 4-CPU x86_64 guest, kernel vfs.all
at 2c3ba1f1cb41 with CONFIG_DEBUG_LIST, PROVE_LOCKING and kmemleak.

A thought, not a patch

The NULL is observable only because the buffer is unlinked and then
relinked. Keeping b_mmb set while the buffer merely moves between
mmb->list and tmp (list_move() instead of __remove_assoc_queue() plus
list_add(), clearing b_mmb only when the buffer is actually dropped as
clean and unlocked) would leave mmb_mark_buffer_dirty() nothing to
race with inside mmb_sync(); the lockless check would then see either
the old mmb or none, never a transient NULL on a buffer that is about
to come back. I have not written or tested that, and you know what
else looks at b_mmb.

If a fix is wanted, we can run it against the module as it was before
our change, on the same guest: that is where the race showed, and the
same test passes 24 of 24 trials with the shared buffers on one list.

Aurelien