| Message ID | 20221201160724.2593341-1-cccheng@synology.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: refuse to remount read-write with unsupported compat-ro features | expand |
On Thu, Dec 1, 2022 at 5:07 PM Chung-Chiang Cheng <cccheng@synology.com> wrote: > > btrfs with unsupported compat-ro features can only be mounted as > read-only, but we can make it read-write indirectly through remount. > Just add the missing check to refuse it. > > mount -o ro /dev/vdb /mnt > mount -o remount,rw /mnt > > Reported-by: Johnny Chang <johnnyc@synology.com> > Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com> > --- > fs/btrfs/super.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index 5942b9384088..45836a426499 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -1991,6 +1991,8 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data) > unsigned old_flags = sb->s_flags; > unsigned long old_opts = fs_info->mount_opt; > unsigned long old_compress_type = fs_info->compress_type; > + u64 compat_ro = btrfs_super_compat_ro_flags(fs_info->super_copy); > + u64 compat_ro_unsupp = compat_ro & ~BTRFS_FEATURE_COMPAT_RO_SUPP; > u64 old_max_inline = fs_info->max_inline; > u32 old_thread_pool_size = fs_info->thread_pool_size; > u32 old_metadata_ratio = fs_info->metadata_ratio; > @@ -2107,6 +2109,13 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data) > if (ret) > goto restore; > } else { > + if (compat_ro_unsupp) { > + btrfs_err(fs_info, > + "cannot remount read-write because of unknown compat_ro features (0x%llx)", > + compat_ro); > + ret = -EINVAL; > + goto restore; > + } Wasn't this already done by the following commit? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=81d5d61454c365718655cfc87d8200c84e25d596 Thanks. > if (BTRFS_FS_ERROR(fs_info)) { > btrfs_err(fs_info, > "Remounting read-write after error is not allowed"); > -- > 2.34.1 >
On Mon, Dec 5, 2022 at 6:45 PM Filipe Manana <fdmanana@kernel.org> wrote: > > Wasn't this already done by the following commit? > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=81d5d61454c365718655cfc87d8200c84e25d596 > > Thanks. > Wow. I did not notice this commit doing the same job by Qu. But I have tested the latest linux-6.1 rc-7, and it's still able to mount a unsupported comat-ro btrfs as read-write via remount. It's caused by the follow-up commit d7f67ac9a928 ("btrfs: relax block-group-tree feature dependency checks"). This commit checks read- only with the current superblock, which will always pass in the situation remounting from read-only to read-write. It seems `btrfs_check_features()` cannot cover this scenario. if (compat_ro_unsupp && !sb_rdonly(sb)) { ^^^^^^^^^^^^^^ Thanks, C.C.Cheng
On Tue, Dec 6, 2022 at 2:42 AM Chung-Chiang Cheng <shepjeng@gmail.com> wrote: > > On Mon, Dec 5, 2022 at 6:45 PM Filipe Manana <fdmanana@kernel.org> wrote: > > > > Wasn't this already done by the following commit? > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=81d5d61454c365718655cfc87d8200c84e25d596 > > > > Thanks. > > > > Wow. I did not notice this commit doing the same job by Qu. But I have > tested the latest linux-6.1 rc-7, and it's still able to mount a unsupported > comat-ro btrfs as read-write via remount. > > It's caused by the follow-up commit d7f67ac9a928 ("btrfs: relax > block-group-tree feature dependency checks"). This commit checks read- > only with the current superblock, which will always pass in the situation > remounting from read-only to read-write. It seems `btrfs_check_features()` > cannot cover this scenario. > > if (compat_ro_unsupp && !sb_rdonly(sb)) { > ^^^^^^^^^^^^^^ Yep, that's a bug. btrfs_check_features() is called before the read only flag is updated in the super block. So the condition should be: if (compat_ro_unsupp && sb_rdonly(sb) && we_want_to_transition_to_rw) We need to pass the flags passed to btrfs_remount() to btrfs_check_features() for that "we_want_to_transition_to_rw" check. That seems to be what needs to be fixed. Thanks. > > Thanks, > C.C.Cheng
On Tue, Dec 06, 2022 at 11:14:56AM +0000, Filipe Manana wrote: > On Tue, Dec 6, 2022 at 2:42 AM Chung-Chiang Cheng <shepjeng@gmail.com> wrote: > > > > On Mon, Dec 5, 2022 at 6:45 PM Filipe Manana <fdmanana@kernel.org> wrote: > > > > > > Wasn't this already done by the following commit? > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=81d5d61454c365718655cfc87d8200c84e25d596 > > > > > > Thanks. > > > > > > > Wow. I did not notice this commit doing the same job by Qu. But I have > > tested the latest linux-6.1 rc-7, and it's still able to mount a unsupported > > comat-ro btrfs as read-write via remount. > > > > It's caused by the follow-up commit d7f67ac9a928 ("btrfs: relax > > block-group-tree feature dependency checks"). This commit checks read- > > only with the current superblock, which will always pass in the situation > > remounting from read-only to read-write. It seems `btrfs_check_features()` > > cannot cover this scenario. > > > > if (compat_ro_unsupp && !sb_rdonly(sb)) { > > ^^^^^^^^^^^^^^ > > Yep, that's a bug. > btrfs_check_features() is called before the read only flag is updated > in the super block. > > So the condition should be: > > if (compat_ro_unsupp && sb_rdonly(sb) && we_want_to_transition_to_rw) > > We need to pass the flags passed to btrfs_remount() to > btrfs_check_features() for that "we_want_to_transition_to_rw" check. > > That seems to be what needs to be fixed. Qu, can you please have a look, it's caused by commit 81d5d61454c365718655cfc87d8200c84e25d596 that's also in stable kernels so this should be fixed quickly.
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index 5942b9384088..45836a426499 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -1991,6 +1991,8 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data) unsigned old_flags = sb->s_flags; unsigned long old_opts = fs_info->mount_opt; unsigned long old_compress_type = fs_info->compress_type; + u64 compat_ro = btrfs_super_compat_ro_flags(fs_info->super_copy); + u64 compat_ro_unsupp = compat_ro & ~BTRFS_FEATURE_COMPAT_RO_SUPP; u64 old_max_inline = fs_info->max_inline; u32 old_thread_pool_size = fs_info->thread_pool_size; u32 old_metadata_ratio = fs_info->metadata_ratio; @@ -2107,6 +2109,13 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data) if (ret) goto restore; } else { + if (compat_ro_unsupp) { + btrfs_err(fs_info, + "cannot remount read-write because of unknown compat_ro features (0x%llx)", + compat_ro); + ret = -EINVAL; + goto restore; + } if (BTRFS_FS_ERROR(fs_info)) { btrfs_err(fs_info, "Remounting read-write after error is not allowed");
btrfs with unsupported compat-ro features can only be mounted as read-only, but we can make it read-write indirectly through remount. Just add the missing check to refuse it. mount -o ro /dev/vdb /mnt mount -o remount,rw /mnt Reported-by: Johnny Chang <johnnyc@synology.com> Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com> --- fs/btrfs/super.c | 9 +++++++++ 1 file changed, 9 insertions(+)