Re: [PATCH] btrfs: verify the tranisd of the to-be-written dirty extent buffer

[Nikolay Borisov <nborisov@suse.com>]

On 2.03.22 г. 3:10 ч., Qu Wenruo wrote:
> [BUG]
> There is a bug report that a bitflip in the transid part of an extent
> buffer makes btrfs to reject certain tree blocks:
> 
>    BTRFS error (device dm-0): parent transid verify failed on 1382301696 wanted 262166 found 22
> 
> [CAUSE]
> Note the failed transid check, hex(262166) = 0x40016, while
> hex(22) = 0x16.
> 
> It's an obvious bitflip.
> 
> Furthermore, the reporter also confirmed the bitflip is from the
> hardware, so it's a real hardware caused bitflip, and such problem can
> not be detected by the existing tree-checker framework.
> 
> As tree-checker can only verify the content inside one tree block, while
> generation of a tree block can only be verified against its parent.
> 
> So such problem remain undetected.
> 
> [FIX]
> Although tree-checker can not verify it at write-time, we still have a
> quick (but not the most accurate) way to catch such obvious corruption.
> 
> Function csum_one_extent_buffer() is called before we submit metadata
> write.
> 
> Thus it means, all the extent buffer passed in should be dirty tree
> blocks, and should be newer than last committed transaction.
> 
> Using that we can catch the above bitflip.
> 
> Although it's not a perfect solution, as if the corrupted generation is
> higher than the correct value, we have no way to catch it at all.
> 
> Reported-by: Christoph Anton Mitterer <calestyo@scientia.org>
> Link: https://lore.kernel.org/linux-btrfs/2dfcbc130c55cc6fd067b93752e90bd2b079baca.camel@scientia.org/
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/btrfs/disk-io.c | 26 ++++++++++++++++++++------
>   1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index b6a81c39d5f4..a89aa523413b 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -441,17 +441,31 @@ static int csum_one_extent_buffer(struct extent_buffer *eb)
>   	else
>   		ret = btrfs_check_leaf_full(eb);
>   
> -	if (ret < 0) {
> -		btrfs_print_tree(eb, 0);
> +	if (ret < 0)
> +		goto error;
> +
> +	/*
> +	 * Also check the generation, the eb reached here must be newer than
> +	 * last committed. Or something seriously wrong happened.
> +	 */
> +	if (btrfs_header_generation(eb) <= fs_info->last_trans_committed) {
> +		ret = -EUCLEAN;
>   		btrfs_err(fs_info,
> -			"block=%llu write time tree block corruption detected",
> -			eb->start);
> -		WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
> -		return ret;
> +			"block=%llu bad generation, have %llu expect > %llu",
> +			  eb->start, btrfs_header_generation(eb),
> +			  fs_info->last_trans_committed);
> +		goto error;

nit: I'd rather have this check in btrfs_check_node/check_leaf functions 
rather than having just this specific check in csum_one_extent_buffer. 
The only thing which is missing AFAICS is the fact the check function 
don't have a context whether we are checking for read or for write. It 
might make sense to extend them to get a boolean param whether the 
validation is for a write or not ?

>   	}
>   	write_extent_buffer(eb, result, 0, fs_info->csum_size);
>   
>   	return 0;
> +error:
> +	btrfs_print_tree(eb, 0);
> +	btrfs_err(fs_info,
> +		"block=%llu write time tree block corruption detected",
> +		eb->start);
> +	WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
> +	return ret;
>   }
>   
>   /* Checksum all dirty extent buffers in one bio_vec */