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@xxxxxxx>
> Link: https://lore.kernel.org/linux-fsdevel/q472nkbfcfdumbxm3cebfnnmueqyl6uhhnper5t6e2cxajpvtu@7prjjo7k6ia3/
> Assisted-by: LLM
> Signed-off-by: Aurelien DESBRIERES <aurelien@xxxxxxxxxxxx>

Thanks! Looks good! Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

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