| Message ID | 8ab95b9b1469cf773928e720a9d787316dfb44bc.1671577140.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix compat_ro checks against remount | expand |
On 12/21/22 07:16, Qu Wenruo wrote: > [BUG] > Even with commit 81d5d61454c3 ("btrfs: enhance unsupported compat RO > flags handling"), btrfs can still mount a fs with unsupported compat_ro > flags read-only, then remount it RW: > > # btrfs ins dump-super /dev/loop0 | grep compat_ro_flags -A 3 > compat_ro_flags 0x403 > ( FREE_SPACE_TREE | > FREE_SPACE_TREE_VALID | > unknown flag: 0x400 ) I am trying to understand how the value 'unknown flag: 0x400' was obtained for the 'compat_ro_flags' variable. A crafted 'sb'? > > # mount /dev/loop0 /mnt/btrfs > mount: /mnt/btrfs: wrong fs type, bad option, bad superblock on /dev/loop0, missing codepage or helper program, or other error. > dmesg(1) may have more information after failed mount system call. > ^^^ RW mount failed as expected ^^^ > > # dmesg -t | tail -n5 > loop0: detected capacity change from 0 to 1048576 > BTRFS: device fsid cb5b82f5-0fdd-4d81-9b4b-78533c324afa devid 1 transid 7 /dev/loop0 scanned by mount (1146) > BTRFS info (device loop0): using crc32c (crc32c-intel) checksum algorithm > BTRFS info (device loop0): using free space tree > BTRFS error (device loop0): cannot mount read-write because of unknown compat_ro features (0x403) > BTRFS error (device loop0): open_ctree failed > > # mount /dev/loop0 -o ro /mnt/btrfs > # mount -o remount,rw /mnt/btrfs > ^^^ RW remount succeeded unexpectedly ^^^ > > [CAUSE] > Currently we use btrfs_check_features() to check compat_ro flags against > our current moount flags. > > That function get reused between open_ctree() and btrfs_remount(). > > But for btrfs_remount(), the super block we passed in still has the old > mount flags, thus btrfs_check_features() still believes we're mounting > read-only. > > [FIX] > Introduce a new argument, *new_flags, to indicate the new mount flags. > That argument can be NULL for the open_ctree() call site. > > With that new argument, call site at btrfs_remount() can properly pass > the new super flags, and we can reject the RW remount correctly. > > Reported-by: Chung-Chiang Cheng <shepjeng@gmail.com> > Fixes: 81d5d61454c3 ("btrfs: enhance unsupported compat RO flags handling") > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/disk-io.c | 9 ++++++--- > fs/btrfs/disk-io.h | 3 ++- > fs/btrfs/super.c | 2 +- > 3 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 0888d484df80..210363db3e2d 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -3391,12 +3391,15 @@ int btrfs_start_pre_rw_mount(struct btrfs_fs_info *fs_info) > * (space cache related) can modify on-disk format like free space tree and > * screw up certain feature dependencies. > */ > -int btrfs_check_features(struct btrfs_fs_info *fs_info, struct super_block *sb) > +int btrfs_check_features(struct btrfs_fs_info *fs_info, struct super_block *sb, > + int *new_flags) > { > struct btrfs_super_block *disk_super = fs_info->super_copy; > u64 incompat = btrfs_super_incompat_flags(disk_super); > const u64 compat_ro = btrfs_super_compat_ro_flags(disk_super); > const u64 compat_ro_unsupp = (compat_ro & ~BTRFS_FEATURE_COMPAT_RO_SUPP); > + bool rw_mount = (!sb_rdonly(sb) || > + (new_flags && !(*new_flags & SB_RDONLY))); The remount is used to change the state of a mount point from read-only (ro) to read-write (rw), or vice versa. In both of these transitions, the %rw_mount variable remains true. So it is not clear to me, what the intended purpose of this is. -Anand > > if (incompat & ~BTRFS_FEATURE_INCOMPAT_SUPP) { > btrfs_err(fs_info, > @@ -3430,7 +3433,7 @@ int btrfs_check_features(struct btrfs_fs_info *fs_info, struct super_block *sb) > if (btrfs_super_nodesize(disk_super) > PAGE_SIZE) > incompat |= BTRFS_FEATURE_INCOMPAT_BIG_METADATA; > > - if (compat_ro_unsupp && !sb_rdonly(sb)) { > + if (compat_ro_unsupp && rw_mount) { > btrfs_err(fs_info, > "cannot mount read-write because of unknown compat_ro features (0x%llx)", > compat_ro); > @@ -3633,7 +3636,7 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device > goto fail_alloc; > } > > - ret = btrfs_check_features(fs_info, sb); > + ret = btrfs_check_features(fs_info, sb, NULL); > if (ret < 0) { > err = ret; > goto fail_alloc; > diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h > index 363935cfc084..e83453c7c429 100644 > --- a/fs/btrfs/disk-io.h > +++ b/fs/btrfs/disk-io.h > @@ -50,7 +50,8 @@ int __cold open_ctree(struct super_block *sb, > void __cold close_ctree(struct btrfs_fs_info *fs_info); > int btrfs_validate_super(struct btrfs_fs_info *fs_info, > struct btrfs_super_block *sb, int mirror_num); > -int btrfs_check_features(struct btrfs_fs_info *fs_info, struct super_block *sb); > +int btrfs_check_features(struct btrfs_fs_info *fs_info, struct super_block *sb, > + int *new_flags); > int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors); > struct btrfs_super_block *btrfs_read_dev_super(struct block_device *bdev); > struct btrfs_super_block *btrfs_read_dev_one_super(struct block_device *bdev, > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index d5de18d6517e..bc2094aa9a40 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -1705,7 +1705,7 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data) > if (ret) > goto restore; > > - ret = btrfs_check_features(fs_info, sb); > + ret = btrfs_check_features(fs_info, sb, flags); > if (ret < 0) > goto restore; >
On 2022/12/21 15:56, Anand Jain wrote: > On 12/21/22 07:16, Qu Wenruo wrote: >> [BUG] >> Even with commit 81d5d61454c3 ("btrfs: enhance unsupported compat RO >> flags handling"), btrfs can still mount a fs with unsupported compat_ro >> flags read-only, then remount it RW: >> >> # btrfs ins dump-super /dev/loop0 | grep compat_ro_flags -A 3 >> compat_ro_flags 0x403 >> ( FREE_SPACE_TREE | >> FREE_SPACE_TREE_VALID | >> unknown flag: 0x400 ) > > > I am trying to understand how the value 'unknown flag: 0x400' was > obtained for the 'compat_ro_flags' variable. A crafted 'sb'? Yes, crafted super block. > > >> >> # mount /dev/loop0 /mnt/btrfs >> mount: /mnt/btrfs: wrong fs type, bad option, bad superblock on >> /dev/loop0, missing codepage or helper program, or other error. >> dmesg(1) may have more information after failed mount system >> call. >> ^^^ RW mount failed as expected ^^^ >> >> # dmesg -t | tail -n5 >> loop0: detected capacity change from 0 to 1048576 >> BTRFS: device fsid cb5b82f5-0fdd-4d81-9b4b-78533c324afa devid 1 >> transid 7 /dev/loop0 scanned by mount (1146) >> BTRFS info (device loop0): using crc32c (crc32c-intel) checksum >> algorithm >> BTRFS info (device loop0): using free space tree >> BTRFS error (device loop0): cannot mount read-write because of >> unknown compat_ro features (0x403) >> BTRFS error (device loop0): open_ctree failed >> >> # mount /dev/loop0 -o ro /mnt/btrfs >> # mount -o remount,rw /mnt/btrfs >> ^^^ RW remount succeeded unexpectedly ^^^ >> >> [CAUSE] >> Currently we use btrfs_check_features() to check compat_ro flags against >> our current moount flags. >> >> That function get reused between open_ctree() and btrfs_remount(). >> >> But for btrfs_remount(), the super block we passed in still has the old >> mount flags, thus btrfs_check_features() still believes we're mounting >> read-only. >> >> [FIX] >> Introduce a new argument, *new_flags, to indicate the new mount flags. >> That argument can be NULL for the open_ctree() call site. >> >> With that new argument, call site at btrfs_remount() can properly pass >> the new super flags, and we can reject the RW remount correctly. >> >> Reported-by: Chung-Chiang Cheng <shepjeng@gmail.com> >> Fixes: 81d5d61454c3 ("btrfs: enhance unsupported compat RO flags >> handling") >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/disk-io.c | 9 ++++++--- >> fs/btrfs/disk-io.h | 3 ++- >> fs/btrfs/super.c | 2 +- >> 3 files changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >> index 0888d484df80..210363db3e2d 100644 >> --- a/fs/btrfs/disk-io.c >> +++ b/fs/btrfs/disk-io.c >> @@ -3391,12 +3391,15 @@ int btrfs_start_pre_rw_mount(struct >> btrfs_fs_info *fs_info) >> * (space cache related) can modify on-disk format like free space >> tree and >> * screw up certain feature dependencies. >> */ >> -int btrfs_check_features(struct btrfs_fs_info *fs_info, struct >> super_block *sb) >> +int btrfs_check_features(struct btrfs_fs_info *fs_info, struct >> super_block *sb, >> + int *new_flags) >> { >> struct btrfs_super_block *disk_super = fs_info->super_copy; >> u64 incompat = btrfs_super_incompat_flags(disk_super); >> const u64 compat_ro = btrfs_super_compat_ro_flags(disk_super); >> const u64 compat_ro_unsupp = (compat_ro & >> ~BTRFS_FEATURE_COMPAT_RO_SUPP); > > >> + bool rw_mount = (!sb_rdonly(sb) || >> + (new_flags && !(*new_flags & SB_RDONLY))); > > The remount is used to change the state of a mount point from > read-only (ro) to read-write (rw), or vice versa. In both of these > transitions, the %rw_mount variable remains true. So it is not clear > to me, what the intended purpose of this is. If rw_mount is already true, the fs doesn't has unsupported compat_flag anyway, thus we don't really need to bother the unsupported flags. The only case rw_mount is true and we care is, RO->RW remount. > > -Anand > >> if (incompat & ~BTRFS_FEATURE_INCOMPAT_SUPP) { >> btrfs_err(fs_info, >> @@ -3430,7 +3433,7 @@ int btrfs_check_features(struct btrfs_fs_info >> *fs_info, struct super_block *sb) >> if (btrfs_super_nodesize(disk_super) > PAGE_SIZE) >> incompat |= BTRFS_FEATURE_INCOMPAT_BIG_METADATA; >> - if (compat_ro_unsupp && !sb_rdonly(sb)) { >> + if (compat_ro_unsupp && rw_mount) { >> btrfs_err(fs_info, >> "cannot mount read-write because of unknown compat_ro features >> (0x%llx)", >> compat_ro); >> @@ -3633,7 +3636,7 @@ int __cold open_ctree(struct super_block *sb, >> struct btrfs_fs_devices *fs_device >> goto fail_alloc; >> } >> - ret = btrfs_check_features(fs_info, sb); >> + ret = btrfs_check_features(fs_info, sb, NULL); >> if (ret < 0) { >> err = ret; >> goto fail_alloc; >> diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h >> index 363935cfc084..e83453c7c429 100644 >> --- a/fs/btrfs/disk-io.h >> +++ b/fs/btrfs/disk-io.h >> @@ -50,7 +50,8 @@ int __cold open_ctree(struct super_block *sb, >> void __cold close_ctree(struct btrfs_fs_info *fs_info); >> int btrfs_validate_super(struct btrfs_fs_info *fs_info, >> struct btrfs_super_block *sb, int mirror_num); >> -int btrfs_check_features(struct btrfs_fs_info *fs_info, struct >> super_block *sb); >> +int btrfs_check_features(struct btrfs_fs_info *fs_info, struct >> super_block *sb, >> + int *new_flags); >> int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors); >> struct btrfs_super_block *btrfs_read_dev_super(struct block_device >> *bdev); >> struct btrfs_super_block *btrfs_read_dev_one_super(struct >> block_device *bdev, >> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c >> index d5de18d6517e..bc2094aa9a40 100644 >> --- a/fs/btrfs/super.c >> +++ b/fs/btrfs/super.c >> @@ -1705,7 +1705,7 @@ static int btrfs_remount(struct super_block *sb, >> int *flags, char *data) >> if (ret) >> goto restore; >> - ret = btrfs_check_features(fs_info, sb); >> + ret = btrfs_check_features(fs_info, sb, flags); >> if (ret < 0) >> goto restore; >
On 12/21/22 15:59, Qu Wenruo wrote: > > > On 2022/12/21 15:56, Anand Jain wrote: >> On 12/21/22 07:16, Qu Wenruo wrote: >>> [BUG] >>> Even with commit 81d5d61454c3 ("btrfs: enhance unsupported compat RO >>> flags handling"), btrfs can still mount a fs with unsupported compat_ro >>> flags read-only, then remount it RW: >>> >>> # btrfs ins dump-super /dev/loop0 | grep compat_ro_flags -A 3 >>> compat_ro_flags 0x403 >>> ( FREE_SPACE_TREE | >>> FREE_SPACE_TREE_VALID | >>> unknown flag: 0x400 ) >> >> >> I am trying to understand how the value 'unknown flag: 0x400' was >> obtained for the 'compat_ro_flags' variable. A crafted 'sb'? > > Yes, crafted super block. > >> >> >>> >>> # mount /dev/loop0 /mnt/btrfs >>> mount: /mnt/btrfs: wrong fs type, bad option, bad superblock on >>> /dev/loop0, missing codepage or helper program, or other error. >>> dmesg(1) may have more information after failed mount >>> system call. >>> ^^^ RW mount failed as expected ^^^ >>> >>> # dmesg -t | tail -n5 >>> loop0: detected capacity change from 0 to 1048576 >>> BTRFS: device fsid cb5b82f5-0fdd-4d81-9b4b-78533c324afa devid 1 >>> transid 7 /dev/loop0 scanned by mount (1146) >>> BTRFS info (device loop0): using crc32c (crc32c-intel) checksum >>> algorithm >>> BTRFS info (device loop0): using free space tree >>> BTRFS error (device loop0): cannot mount read-write because of >>> unknown compat_ro features (0x403) >>> BTRFS error (device loop0): open_ctree failed >>> >>> # mount /dev/loop0 -o ro /mnt/btrfs >>> # mount -o remount,rw /mnt/btrfs >>> ^^^ RW remount succeeded unexpectedly ^^^ >>> >>> [CAUSE] >>> Currently we use btrfs_check_features() to check compat_ro flags against >>> our current moount flags. >>> >>> That function get reused between open_ctree() and btrfs_remount(). >>> >>> But for btrfs_remount(), the super block we passed in still has the old >>> mount flags, thus btrfs_check_features() still believes we're mounting >>> read-only. >>> >>> [FIX] >>> Introduce a new argument, *new_flags, to indicate the new mount flags. >>> That argument can be NULL for the open_ctree() call site. >>> >>> With that new argument, call site at btrfs_remount() can properly pass >>> the new super flags, and we can reject the RW remount correctly. >>> >>> Reported-by: Chung-Chiang Cheng <shepjeng@gmail.com> >>> Fixes: 81d5d61454c3 ("btrfs: enhance unsupported compat RO flags >>> handling") >>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>> --- >>> fs/btrfs/disk-io.c | 9 ++++++--- >>> fs/btrfs/disk-io.h | 3 ++- >>> fs/btrfs/super.c | 2 +- >>> 3 files changed, 9 insertions(+), 5 deletions(-) >>> >>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >>> index 0888d484df80..210363db3e2d 100644 >>> --- a/fs/btrfs/disk-io.c >>> +++ b/fs/btrfs/disk-io.c >>> @@ -3391,12 +3391,15 @@ int btrfs_start_pre_rw_mount(struct >>> btrfs_fs_info *fs_info) >>> * (space cache related) can modify on-disk format like free space >>> tree and >>> * screw up certain feature dependencies. >>> */ >>> -int btrfs_check_features(struct btrfs_fs_info *fs_info, struct >>> super_block *sb) >>> +int btrfs_check_features(struct btrfs_fs_info *fs_info, struct >>> super_block *sb, >>> + int *new_flags) >>> { >>> struct btrfs_super_block *disk_super = fs_info->super_copy; >>> u64 incompat = btrfs_super_incompat_flags(disk_super); >>> const u64 compat_ro = btrfs_super_compat_ro_flags(disk_super); >>> const u64 compat_ro_unsupp = (compat_ro & >>> ~BTRFS_FEATURE_COMPAT_RO_SUPP); >> >> >>> + bool rw_mount = (!sb_rdonly(sb) || >>> + (new_flags && !(*new_flags & SB_RDONLY))); >> >> The remount is used to change the state of a mount point from >> read-only (ro) to read-write (rw), or vice versa. In both of these >> transitions, the %rw_mount variable remains true. So it is not clear >> to me, what the intended purpose of this is. > > If rw_mount is already true, the fs doesn't has unsupported compat_flag > anyway, thus we don't really need to bother the unsupported flags. > > The only case rw_mount is true and we care is, RO->RW remount. Have you tested the value of %rw_mount in the secnarios RO->RW and RW->RW? I did. I find %rw_mount is true in both the cases. >> -Anand >> >>> if (incompat & ~BTRFS_FEATURE_INCOMPAT_SUPP) { >>> btrfs_err(fs_info, >>> @@ -3430,7 +3433,7 @@ int btrfs_check_features(struct btrfs_fs_info >>> *fs_info, struct super_block *sb) >>> if (btrfs_super_nodesize(disk_super) > PAGE_SIZE) >>> incompat |= BTRFS_FEATURE_INCOMPAT_BIG_METADATA; >>> - if (compat_ro_unsupp && !sb_rdonly(sb)) { >>> + if (compat_ro_unsupp && rw_mount) { >>> btrfs_err(fs_info, >>> "cannot mount read-write because of unknown compat_ro features >>> (0x%llx)", >>> compat_ro); >>> @@ -3633,7 +3636,7 @@ int __cold open_ctree(struct super_block *sb, >>> struct btrfs_fs_devices *fs_device >>> goto fail_alloc; >>> } >>> - ret = btrfs_check_features(fs_info, sb); >>> + ret = btrfs_check_features(fs_info, sb, NULL); >>> if (ret < 0) { >>> err = ret; >>> goto fail_alloc; >>> diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h >>> index 363935cfc084..e83453c7c429 100644 >>> --- a/fs/btrfs/disk-io.h >>> +++ b/fs/btrfs/disk-io.h >>> @@ -50,7 +50,8 @@ int __cold open_ctree(struct super_block *sb, >>> void __cold close_ctree(struct btrfs_fs_info *fs_info); >>> int btrfs_validate_super(struct btrfs_fs_info *fs_info, >>> struct btrfs_super_block *sb, int mirror_num); >>> -int btrfs_check_features(struct btrfs_fs_info *fs_info, struct >>> super_block *sb); >>> +int btrfs_check_features(struct btrfs_fs_info *fs_info, struct >>> super_block *sb, >>> + int *new_flags); >>> int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors); >>> struct btrfs_super_block *btrfs_read_dev_super(struct block_device >>> *bdev); >>> struct btrfs_super_block *btrfs_read_dev_one_super(struct >>> block_device *bdev, >>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c >>> index d5de18d6517e..bc2094aa9a40 100644 >>> --- a/fs/btrfs/super.c >>> +++ b/fs/btrfs/super.c >>> @@ -1705,7 +1705,7 @@ static int btrfs_remount(struct super_block >>> *sb, int *flags, char *data) >>> if (ret) >>> goto restore; >>> - ret = btrfs_check_features(fs_info, sb); >>> + ret = btrfs_check_features(fs_info, sb, flags); >>> if (ret < 0) >>> goto restore; >>
On 2022/12/21 16:08, Anand Jain wrote: > > > On 12/21/22 15:59, Qu Wenruo wrote: >> >> >> On 2022/12/21 15:56, Anand Jain wrote: >>> On 12/21/22 07:16, Qu Wenruo wrote: >>>> [BUG] >>>> Even with commit 81d5d61454c3 ("btrfs: enhance unsupported compat RO >>>> flags handling"), btrfs can still mount a fs with unsupported compat_ro >>>> flags read-only, then remount it RW: >>>> >>>> # btrfs ins dump-super /dev/loop0 | grep compat_ro_flags -A 3 >>>> compat_ro_flags 0x403 >>>> ( FREE_SPACE_TREE | >>>> FREE_SPACE_TREE_VALID | >>>> unknown flag: 0x400 ) >>> >>> >>> I am trying to understand how the value 'unknown flag: 0x400' was >>> obtained for the 'compat_ro_flags' variable. A crafted 'sb'? >> >> Yes, crafted super block. >> >>> >>> >>>> >>>> # mount /dev/loop0 /mnt/btrfs >>>> mount: /mnt/btrfs: wrong fs type, bad option, bad superblock on >>>> /dev/loop0, missing codepage or helper program, or other error. >>>> dmesg(1) may have more information after failed mount >>>> system call. >>>> ^^^ RW mount failed as expected ^^^ >>>> >>>> # dmesg -t | tail -n5 >>>> loop0: detected capacity change from 0 to 1048576 >>>> BTRFS: device fsid cb5b82f5-0fdd-4d81-9b4b-78533c324afa devid 1 >>>> transid 7 /dev/loop0 scanned by mount (1146) >>>> BTRFS info (device loop0): using crc32c (crc32c-intel) checksum >>>> algorithm >>>> BTRFS info (device loop0): using free space tree >>>> BTRFS error (device loop0): cannot mount read-write because of >>>> unknown compat_ro features (0x403) >>>> BTRFS error (device loop0): open_ctree failed >>>> >>>> # mount /dev/loop0 -o ro /mnt/btrfs >>>> # mount -o remount,rw /mnt/btrfs >>>> ^^^ RW remount succeeded unexpectedly ^^^ >>>> >>>> [CAUSE] >>>> Currently we use btrfs_check_features() to check compat_ro flags >>>> against >>>> our current moount flags. >>>> >>>> That function get reused between open_ctree() and btrfs_remount(). >>>> >>>> But for btrfs_remount(), the super block we passed in still has the old >>>> mount flags, thus btrfs_check_features() still believes we're mounting >>>> read-only. >>>> >>>> [FIX] >>>> Introduce a new argument, *new_flags, to indicate the new mount flags. >>>> That argument can be NULL for the open_ctree() call site. >>>> >>>> With that new argument, call site at btrfs_remount() can properly pass >>>> the new super flags, and we can reject the RW remount correctly. >>>> >>>> Reported-by: Chung-Chiang Cheng <shepjeng@gmail.com> >>>> Fixes: 81d5d61454c3 ("btrfs: enhance unsupported compat RO flags >>>> handling") >>>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>>> --- >>>> fs/btrfs/disk-io.c | 9 ++++++--- >>>> fs/btrfs/disk-io.h | 3 ++- >>>> fs/btrfs/super.c | 2 +- >>>> 3 files changed, 9 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >>>> index 0888d484df80..210363db3e2d 100644 >>>> --- a/fs/btrfs/disk-io.c >>>> +++ b/fs/btrfs/disk-io.c >>>> @@ -3391,12 +3391,15 @@ int btrfs_start_pre_rw_mount(struct >>>> btrfs_fs_info *fs_info) >>>> * (space cache related) can modify on-disk format like free space >>>> tree and >>>> * screw up certain feature dependencies. >>>> */ >>>> -int btrfs_check_features(struct btrfs_fs_info *fs_info, struct >>>> super_block *sb) >>>> +int btrfs_check_features(struct btrfs_fs_info *fs_info, struct >>>> super_block *sb, >>>> + int *new_flags) >>>> { >>>> struct btrfs_super_block *disk_super = fs_info->super_copy; >>>> u64 incompat = btrfs_super_incompat_flags(disk_super); >>>> const u64 compat_ro = btrfs_super_compat_ro_flags(disk_super); >>>> const u64 compat_ro_unsupp = (compat_ro & >>>> ~BTRFS_FEATURE_COMPAT_RO_SUPP); >>> >>> >>>> + bool rw_mount = (!sb_rdonly(sb) || >>>> + (new_flags && !(*new_flags & SB_RDONLY))); >>> >>> The remount is used to change the state of a mount point from >>> read-only (ro) to read-write (rw), or vice versa. In both of these >>> transitions, the %rw_mount variable remains true. So it is not clear >>> to me, what the intended purpose of this is. >> >> If rw_mount is already true, the fs doesn't has unsupported >> compat_flag anyway, thus we don't really need to bother the >> unsupported flags. >> > >> The only case rw_mount is true and we care is, RO->RW remount. > > Have you tested the value of %rw_mount in the secnarios RO->RW > and RW->RW? I did. I find %rw_mount is true in both the cases. What's the problem? That's exactly the expected behavior. We don't want to allow RO->RW with unsupported compat_ro flag. The truth table is very simple: rw_mount RO->RO False RO->RW True RW->RW True RW->RO True And in above cases, RO->RW is what we care. RW->RW is fine, as if the fs is already mounted RW, the compat_ro flags must be fine. RW->RO is the same as RW->RW. So, what's your problem here? Thanks, Qu > > > >>> -Anand >>> >>>> if (incompat & ~BTRFS_FEATURE_INCOMPAT_SUPP) { >>>> btrfs_err(fs_info, >>>> @@ -3430,7 +3433,7 @@ int btrfs_check_features(struct btrfs_fs_info >>>> *fs_info, struct super_block *sb) >>>> if (btrfs_super_nodesize(disk_super) > PAGE_SIZE) >>>> incompat |= BTRFS_FEATURE_INCOMPAT_BIG_METADATA; >>>> - if (compat_ro_unsupp && !sb_rdonly(sb)) { >>>> + if (compat_ro_unsupp && rw_mount) { >>>> btrfs_err(fs_info, >>>> "cannot mount read-write because of unknown compat_ro features >>>> (0x%llx)", >>>> compat_ro); >>>> @@ -3633,7 +3636,7 @@ int __cold open_ctree(struct super_block *sb, >>>> struct btrfs_fs_devices *fs_device >>>> goto fail_alloc; >>>> } >>>> - ret = btrfs_check_features(fs_info, sb); >>>> + ret = btrfs_check_features(fs_info, sb, NULL); >>>> if (ret < 0) { >>>> err = ret; >>>> goto fail_alloc; >>>> diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h >>>> index 363935cfc084..e83453c7c429 100644 >>>> --- a/fs/btrfs/disk-io.h >>>> +++ b/fs/btrfs/disk-io.h >>>> @@ -50,7 +50,8 @@ int __cold open_ctree(struct super_block *sb, >>>> void __cold close_ctree(struct btrfs_fs_info *fs_info); >>>> int btrfs_validate_super(struct btrfs_fs_info *fs_info, >>>> struct btrfs_super_block *sb, int mirror_num); >>>> -int btrfs_check_features(struct btrfs_fs_info *fs_info, struct >>>> super_block *sb); >>>> +int btrfs_check_features(struct btrfs_fs_info *fs_info, struct >>>> super_block *sb, >>>> + int *new_flags); >>>> int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors); >>>> struct btrfs_super_block *btrfs_read_dev_super(struct block_device >>>> *bdev); >>>> struct btrfs_super_block *btrfs_read_dev_one_super(struct >>>> block_device *bdev, >>>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c >>>> index d5de18d6517e..bc2094aa9a40 100644 >>>> --- a/fs/btrfs/super.c >>>> +++ b/fs/btrfs/super.c >>>> @@ -1705,7 +1705,7 @@ static int btrfs_remount(struct super_block >>>> *sb, int *flags, char *data) >>>> if (ret) >>>> goto restore; >>>> - ret = btrfs_check_features(fs_info, sb); >>>> + ret = btrfs_check_features(fs_info, sb, flags); >>>> if (ret < 0) >>>> goto restore; >>>
On 12/21/22 16:14, Qu Wenruo wrote: > > > On 2022/12/21 16:08, Anand Jain wrote: >> >> >> On 12/21/22 15:59, Qu Wenruo wrote: >>> >>> >>> On 2022/12/21 15:56, Anand Jain wrote: >>>> On 12/21/22 07:16, Qu Wenruo wrote: >>>>> [BUG] >>>>> Even with commit 81d5d61454c3 ("btrfs: enhance unsupported compat RO >>>>> flags handling"), btrfs can still mount a fs with unsupported >>>>> compat_ro >>>>> flags read-only, then remount it RW: >>>>> >>>>> # btrfs ins dump-super /dev/loop0 | grep compat_ro_flags -A 3 >>>>> compat_ro_flags 0x403 >>>>> ( FREE_SPACE_TREE | >>>>> FREE_SPACE_TREE_VALID | >>>>> unknown flag: 0x400 ) >>>> >>>> >>>> I am trying to understand how the value 'unknown flag: 0x400' was >>>> obtained for the 'compat_ro_flags' variable. A crafted 'sb'? >>> >>> Yes, crafted super block. >>> >>>> >>>> >>>>> >>>>> # mount /dev/loop0 /mnt/btrfs >>>>> mount: /mnt/btrfs: wrong fs type, bad option, bad superblock on >>>>> /dev/loop0, missing codepage or helper program, or other error. >>>>> dmesg(1) may have more information after failed mount >>>>> system call. >>>>> ^^^ RW mount failed as expected ^^^ >>>>> >>>>> # dmesg -t | tail -n5 >>>>> loop0: detected capacity change from 0 to 1048576 >>>>> BTRFS: device fsid cb5b82f5-0fdd-4d81-9b4b-78533c324afa devid 1 >>>>> transid 7 /dev/loop0 scanned by mount (1146) >>>>> BTRFS info (device loop0): using crc32c (crc32c-intel) checksum >>>>> algorithm >>>>> BTRFS info (device loop0): using free space tree >>>>> BTRFS error (device loop0): cannot mount read-write because of >>>>> unknown compat_ro features (0x403) >>>>> BTRFS error (device loop0): open_ctree failed >>>>> >>>>> # mount /dev/loop0 -o ro /mnt/btrfs >>>>> # mount -o remount,rw /mnt/btrfs >>>>> ^^^ RW remount succeeded unexpectedly ^^^ >>>>> >>>>> [CAUSE] >>>>> Currently we use btrfs_check_features() to check compat_ro flags >>>>> against >>>>> our current moount flags. >>>>> >>>>> That function get reused between open_ctree() and btrfs_remount(). >>>>> >>>>> But for btrfs_remount(), the super block we passed in still has the >>>>> old >>>>> mount flags, thus btrfs_check_features() still believes we're mounting >>>>> read-only. >>>>> >>>>> [FIX] >>>>> Introduce a new argument, *new_flags, to indicate the new mount flags. >>>>> That argument can be NULL for the open_ctree() call site. >>>>> >>>>> With that new argument, call site at btrfs_remount() can properly pass >>>>> the new super flags, and we can reject the RW remount correctly. >>>>> >>>>> Reported-by: Chung-Chiang Cheng <shepjeng@gmail.com> >>>>> Fixes: 81d5d61454c3 ("btrfs: enhance unsupported compat RO flags >>>>> handling") >>>>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>>>> --- >>>>> fs/btrfs/disk-io.c | 9 ++++++--- >>>>> fs/btrfs/disk-io.h | 3 ++- >>>>> fs/btrfs/super.c | 2 +- >>>>> 3 files changed, 9 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >>>>> index 0888d484df80..210363db3e2d 100644 >>>>> --- a/fs/btrfs/disk-io.c >>>>> +++ b/fs/btrfs/disk-io.c >>>>> @@ -3391,12 +3391,15 @@ int btrfs_start_pre_rw_mount(struct >>>>> btrfs_fs_info *fs_info) >>>>> * (space cache related) can modify on-disk format like free >>>>> space tree and >>>>> * screw up certain feature dependencies. >>>>> */ >>>>> -int btrfs_check_features(struct btrfs_fs_info *fs_info, struct >>>>> super_block *sb) >>>>> +int btrfs_check_features(struct btrfs_fs_info *fs_info, struct >>>>> super_block *sb, >>>>> + int *new_flags) >>>>> { >>>>> struct btrfs_super_block *disk_super = fs_info->super_copy; >>>>> u64 incompat = btrfs_super_incompat_flags(disk_super); >>>>> const u64 compat_ro = btrfs_super_compat_ro_flags(disk_super); >>>>> const u64 compat_ro_unsupp = (compat_ro & >>>>> ~BTRFS_FEATURE_COMPAT_RO_SUPP); >>>> >>>> >>>>> + bool rw_mount = (!sb_rdonly(sb) || >>>>> + (new_flags && !(*new_flags & SB_RDONLY))); >>>> >>>> The remount is used to change the state of a mount point from >>>> read-only (ro) to read-write (rw), or vice versa. In both of these >>>> transitions, the %rw_mount variable remains true. So it is not clear >>>> to me, what the intended purpose of this is. >>> >>> If rw_mount is already true, the fs doesn't has unsupported >>> compat_flag anyway, thus we don't really need to bother the >>> unsupported flags. >>> >> >>> The only case rw_mount is true and we care is, RO->RW remount. Confusing statement; Its not the only case it is true. as in the table below. >> >> Have you tested the value of %rw_mount in the secnarios RO->RW >> and RW->RW? I did. I find %rw_mount is true in both the cases. > > What's the problem? That's exactly the expected behavior. > > We don't want to allow RO->RW with unsupported compat_ro flag. > > The truth table is very simple: > - if (compat_ro_unsupp && !sb_rdonly(sb)) { + if (compat_ro_unsupp && rw_mount) { rw_mount !sb_rdonly(sb) RO->RO False False RO->RW True False RW->RW True True RW->RO True True Okay. Reviewed-by: Anand Jain <anand.jain@oracle.com>
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 0888d484df80..210363db3e2d 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -3391,12 +3391,15 @@ int btrfs_start_pre_rw_mount(struct btrfs_fs_info *fs_info) * (space cache related) can modify on-disk format like free space tree and * screw up certain feature dependencies. */ -int btrfs_check_features(struct btrfs_fs_info *fs_info, struct super_block *sb) +int btrfs_check_features(struct btrfs_fs_info *fs_info, struct super_block *sb, + int *new_flags) { struct btrfs_super_block *disk_super = fs_info->super_copy; u64 incompat = btrfs_super_incompat_flags(disk_super); const u64 compat_ro = btrfs_super_compat_ro_flags(disk_super); const u64 compat_ro_unsupp = (compat_ro & ~BTRFS_FEATURE_COMPAT_RO_SUPP); + bool rw_mount = (!sb_rdonly(sb) || + (new_flags && !(*new_flags & SB_RDONLY))); if (incompat & ~BTRFS_FEATURE_INCOMPAT_SUPP) { btrfs_err(fs_info, @@ -3430,7 +3433,7 @@ int btrfs_check_features(struct btrfs_fs_info *fs_info, struct super_block *sb) if (btrfs_super_nodesize(disk_super) > PAGE_SIZE) incompat |= BTRFS_FEATURE_INCOMPAT_BIG_METADATA; - if (compat_ro_unsupp && !sb_rdonly(sb)) { + if (compat_ro_unsupp && rw_mount) { btrfs_err(fs_info, "cannot mount read-write because of unknown compat_ro features (0x%llx)", compat_ro); @@ -3633,7 +3636,7 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device goto fail_alloc; } - ret = btrfs_check_features(fs_info, sb); + ret = btrfs_check_features(fs_info, sb, NULL); if (ret < 0) { err = ret; goto fail_alloc; diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h index 363935cfc084..e83453c7c429 100644 --- a/fs/btrfs/disk-io.h +++ b/fs/btrfs/disk-io.h @@ -50,7 +50,8 @@ int __cold open_ctree(struct super_block *sb, void __cold close_ctree(struct btrfs_fs_info *fs_info); int btrfs_validate_super(struct btrfs_fs_info *fs_info, struct btrfs_super_block *sb, int mirror_num); -int btrfs_check_features(struct btrfs_fs_info *fs_info, struct super_block *sb); +int btrfs_check_features(struct btrfs_fs_info *fs_info, struct super_block *sb, + int *new_flags); int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors); struct btrfs_super_block *btrfs_read_dev_super(struct block_device *bdev); struct btrfs_super_block *btrfs_read_dev_one_super(struct block_device *bdev, diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index d5de18d6517e..bc2094aa9a40 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -1705,7 +1705,7 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data) if (ret) goto restore; - ret = btrfs_check_features(fs_info, sb); + ret = btrfs_check_features(fs_info, sb, flags); if (ret < 0) goto restore;
[BUG] Even with commit 81d5d61454c3 ("btrfs: enhance unsupported compat RO flags handling"), btrfs can still mount a fs with unsupported compat_ro flags read-only, then remount it RW: # btrfs ins dump-super /dev/loop0 | grep compat_ro_flags -A 3 compat_ro_flags 0x403 ( FREE_SPACE_TREE | FREE_SPACE_TREE_VALID | unknown flag: 0x400 ) # mount /dev/loop0 /mnt/btrfs mount: /mnt/btrfs: wrong fs type, bad option, bad superblock on /dev/loop0, missing codepage or helper program, or other error. dmesg(1) may have more information after failed mount system call. ^^^ RW mount failed as expected ^^^ # dmesg -t | tail -n5 loop0: detected capacity change from 0 to 1048576 BTRFS: device fsid cb5b82f5-0fdd-4d81-9b4b-78533c324afa devid 1 transid 7 /dev/loop0 scanned by mount (1146) BTRFS info (device loop0): using crc32c (crc32c-intel) checksum algorithm BTRFS info (device loop0): using free space tree BTRFS error (device loop0): cannot mount read-write because of unknown compat_ro features (0x403) BTRFS error (device loop0): open_ctree failed # mount /dev/loop0 -o ro /mnt/btrfs # mount -o remount,rw /mnt/btrfs ^^^ RW remount succeeded unexpectedly ^^^ [CAUSE] Currently we use btrfs_check_features() to check compat_ro flags against our current moount flags. That function get reused between open_ctree() and btrfs_remount(). But for btrfs_remount(), the super block we passed in still has the old mount flags, thus btrfs_check_features() still believes we're mounting read-only. [FIX] Introduce a new argument, *new_flags, to indicate the new mount flags. That argument can be NULL for the open_ctree() call site. With that new argument, call site at btrfs_remount() can properly pass the new super flags, and we can reject the RW remount correctly. Reported-by: Chung-Chiang Cheng <shepjeng@gmail.com> Fixes: 81d5d61454c3 ("btrfs: enhance unsupported compat RO flags handling") Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/disk-io.c | 9 ++++++--- fs/btrfs/disk-io.h | 3 ++- fs/btrfs/super.c | 2 +- 3 files changed, 9 insertions(+), 5 deletions(-)