Re: mmb_sync: b_mmb NULL window races mmb_mark_buffer_dirty

[Jan Kara]

Hi!

On Sat 24-10-26 19:06:09, Aurelien DESBRIERES wrote:
> 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.

Well, yes, mmb code is all built on the assumption that each buffer has at
most one inode with which it can be tracked. If the filesystem tries to
attach it to more than one inode, all bets are off and we give no
guarantees what happens. I guess we should add an assertion to
mmb_mark_buffer_dirty() for that to warn about buggy filesystem code early.

So what you did - handling shared buffers separately - is the right fix for
the problem you were hitting.

								Honza

> 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
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR