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

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@suse.com>
SUSE Labs, CR

Re: mmb_sync: b_mmb NULL window races mmb_mark_buffer_dirty

[Aurelien DESBRIERES]

On Mon 26-10-26 14:08:12, Jan Kara wrote:
> 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.

Thanks, that settles it: one buffer, one mmb, and our fix stands.

The assertion in mmb_mark_buffer_dirty() I can send, if you want it
from here: a WARN_ON_ONCE on bh->b_mmb set to another mmb, before the
NULL check. We still have the module that attached shared buffers to
several inodes, so the warning can be shown firing on the same
generic/241 run that corrupted the list.

Aurelien

[PATCH] buffer: warn when a buffer is attached to a second mmb

[Aurelien DESBRIERES]

mmb_mark_buffer_dirty() tracks a buffer on one mapping_metadata_bhs at
most. The per-mmb lock does not protect a buffer that changes lists,
and mmb_sync() clears b_mmb for a moment under its own lock, so a
filesystem that attaches a buffer shared by several inodes to each
inode's list in turn corrupts b_assoc_buffers eventually. With
CONFIG_DEBUG_LIST the list_del is refused and mmb_sync() spins on a
list that never empties, holding mmb->lock: seen with beamfs under
dbench (xfstests generic/241) as list_del corruption, a soft lockup and
a node lost to the OOM killer, with nothing naming the filesystem.

Warn once when a buffer already tracked by another mmb is marked dirty
through a second one, so that the misuse is named at its first
occurrence rather than at the corruption it leads to. A corrupted
volume in which two inodes claim the same block reaches this as well;
the cmpxchg below keeps the lists sound in that case, and the warning
then says which filesystem believed its block map.

Suggested-by: Jan Kara <jack@suse.cz>
Link: https://lore.kernel.org/linux-fsdevel/q472nkbfcfdumbxm3cebfnnmueqyl6uhhnper5t6e2cxajpvtu@7prjjo7k6ia3/
Assisted-by: LLM
Signed-off-by: Aurelien DESBRIERES <aurelien@hackers.camp>
---
Tested on vfs.all with beamfs, x86_64 guest, 4 CPUs:

- the module as it was before our fix, attaching the shared bitmap
  block to each allocating inode's list in turn: the warning fires at
  boot, 8.6 s in, on the first truncate that frees blocks of a file
  another inode had allocated, long before the dbench run that used
  to corrupt the list:

    WARNING: fs/buffer.c:639 at mmb_mark_buffer_dirty+0x46/0xd0, CPU#3: S01hostname-cmd/111
     beamfs_write_bitmap_block+0x191/0x1c0
     beamfs_inline_free_blocks_from+0x64/0x390
     beamfs_inline_setattr+0x127/0x4c0
     notify_change+0x33b/0x520
     do_truncate+0xbf/0xf0
     path_openat+0x8c1/0xaa0

- the module with the shared buffers on one superblock list: no
  warning at boot or under the filesystem's own tests.

 fs/buffer.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fs/buffer.c b/fs/buffer.c
index 1dc933ba6925..e503a7ed9c37 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -627,6 +627,16 @@ void write_boundary_block(struct block_device *bdev,
 void mmb_mark_buffer_dirty(struct buffer_head *bh,
 			   struct mapping_metadata_bhs *mmb)
 {
+	struct mapping_metadata_bhs *cur = READ_ONCE(bh->b_mmb);
+
+	/*
+	 * A buffer is tracked by one mmb at most. The per-mmb lock does
+	 * not cover a buffer that changes lists, and mmb_sync() clears
+	 * b_mmb for a moment under its own lock, so a filesystem that
+	 * attaches a buffer to a second mmb corrupts b_assoc_buffers
+	 * sooner or later. Name it here, before the lists do.
+	 */
+	WARN_ON_ONCE(cur && cur != mmb);
 	mark_buffer_dirty(bh);
 	if (!bh->b_mmb) {
 		spin_lock(&mmb->lock);

base-commit: 2c3ba1f1cb416b81d617b92a24a4bcaa48d574e3
-- 
2.55.0

Re: [PATCH] buffer: warn when a buffer is attached to a second mmb

[Jan Kara]

On Mon 26-10-26 22:36:26, Aurelien DESBRIERES wrote:
> mmb_mark_buffer_dirty() tracks a buffer on one mapping_metadata_bhs at
> most. The per-mmb lock does not protect a buffer that changes lists,
> and mmb_sync() clears b_mmb for a moment under its own lock, so a
> filesystem that attaches a buffer shared by several inodes to each
> inode's list in turn corrupts b_assoc_buffers eventually. With
> CONFIG_DEBUG_LIST the list_del is refused and mmb_sync() spins on a
> list that never empties, holding mmb->lock: seen with beamfs under
> dbench (xfstests generic/241) as list_del corruption, a soft lockup and
> a node lost to the OOM killer, with nothing naming the filesystem.
> 
> Warn once when a buffer already tracked by another mmb is marked dirty
> through a second one, so that the misuse is named at its first
> occurrence rather than at the corruption it leads to. A corrupted
> volume in which two inodes claim the same block reaches this as well;
> the cmpxchg below keeps the lists sound in that case, and the warning
> then says which filesystem believed its block map.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Link: https://lore.kernel.org/linux-fsdevel/q472nkbfcfdumbxm3cebfnnmueqyl6uhhnper5t6e2cxajpvtu@7prjjo7k6ia3/
> Assisted-by: LLM
> Signed-off-by: Aurelien DESBRIERES <aurelien@hackers.camp>

Thanks! Looks good! Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
> Tested on vfs.all with beamfs, x86_64 guest, 4 CPUs:
> 
> - the module as it was before our fix, attaching the shared bitmap
>   block to each allocating inode's list in turn: the warning fires at
>   boot, 8.6 s in, on the first truncate that frees blocks of a file
>   another inode had allocated, long before the dbench run that used
>   to corrupt the list:
> 
>     WARNING: fs/buffer.c:639 at mmb_mark_buffer_dirty+0x46/0xd0, CPU#3: S01hostname-cmd/111
>      beamfs_write_bitmap_block+0x191/0x1c0
>      beamfs_inline_free_blocks_from+0x64/0x390
>      beamfs_inline_setattr+0x127/0x4c0
>      notify_change+0x33b/0x520
>      do_truncate+0xbf/0xf0
>      path_openat+0x8c1/0xaa0
> 
> - the module with the shared buffers on one superblock list: no
>   warning at boot or under the filesystem's own tests.
> 
>  fs/buffer.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 1dc933ba6925..e503a7ed9c37 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -627,6 +627,16 @@ void write_boundary_block(struct block_device *bdev,
>  void mmb_mark_buffer_dirty(struct buffer_head *bh,
>  			   struct mapping_metadata_bhs *mmb)
>  {
> +	struct mapping_metadata_bhs *cur = READ_ONCE(bh->b_mmb);
> +
> +	/*
> +	 * A buffer is tracked by one mmb at most. The per-mmb lock does
> +	 * not cover a buffer that changes lists, and mmb_sync() clears
> +	 * b_mmb for a moment under its own lock, so a filesystem that
> +	 * attaches a buffer to a second mmb corrupts b_assoc_buffers
> +	 * sooner or later. Name it here, before the lists do.
> +	 */
> +	WARN_ON_ONCE(cur && cur != mmb);
>  	mark_buffer_dirty(bh);
>  	if (!bh->b_mmb) {
>  		spin_lock(&mmb->lock);
> 
> base-commit: 2c3ba1f1cb416b81d617b92a24a4bcaa48d574e3
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR