Re: [PATCH] btrfs: reject log replay if there is unsupported RO flag

[Filipe Manana <fdmanana@kernel.org>]

On Tue, Jun 07, 2022 at 10:31:46AM +0800, Qu Wenruo wrote:
> [BUG]
> If we have a btrfs image with dirty log, along with an unsupported RO
> compatible flag:
> 
> log_root		30474240
> ...
> compat_flags		0x0
> compat_ro_flags		0x40000003
> 			( FREE_SPACE_TREE |
> 			  FREE_SPACE_TREE_VALID |
> 			  unknown flag: 0x40000000 )
> 
> Then even if we can only mount it RO, we will still cause metadata
> update for log replay:
> 
>  BTRFS info (device dm-1): flagging fs with big metadata feature
>  BTRFS info (device dm-1): using free space tree
>  BTRFS info (device dm-1): has skinny extents
>  BTRFS info (device dm-1): start tree-log replay
> 
> This is definitely against RO compact flag requirement.
> 
> [CAUSE]
> RO compact flag only forces us to do RO mount, but we will still do log
> replay for plain RO mount.
> 
> Thus this will result us to do log replay and update metadata.
> 
> This can be very problematic for new RO compat flag, for example older
> kernel can not understand v2 cache, and if we allow metadata update on
> RO mount and invalidate/corrupt v2 cache.
> 
> [FIX]
> Just set the nologreplay flag if there is any unsupported RO compact
> flag.
> 
> This will reject log replay no matter if we have dirty log or not, with
> the following message:
> 
>  BTRFS info (device dm-1): disabling log replay due to unsupported ro compat features
> 
> Cc: stable@vger.kernel.org #4.9+
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/disk-io.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index fe309db9f5ff..d06f1a176b5b 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3655,6 +3655,14 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
>  		err = -EINVAL;
>  		goto fail_alloc;
>  	}
> +	/*
> +	 * We have unsupported RO compat features, although RO mounted, we
> +	 * should any metadata write, including the log replay.
> +	 * Or we can screw up whatever the new feature requires.
> +	 */
> +	if (features)
> +		btrfs_set_and_info(fs_info, NOLOGREPLAY,
> +		"disabling log replay due to unsupported ro compat features");

Well, this might be surprising for users.

On mount, it's expected that everything that was fsynced is available.
Yes, there's a message printed informing the logs were not replayed,
but this allows for applications to read stale data.

I think just failing the mount and printing a message telling that the
fs needs to be explicitly mounted with -o nologreplay is less prone to
having stale data being read and used.

Thanks.

>  
>  	if (sectorsize < PAGE_SIZE) {
>  		struct btrfs_subpage_info *subpage_info;
> -- 
> 2.36.1
>