| Message ID | bde2887da38aaa473ca60801b37ac735b3ab2d6d.1709003728.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: qgroup: verify btrfs_qgroup_inherit parameter | expand |
On Tue, Feb 27, 2024 at 01:45:35PM +1030, Qu Wenruo wrote: > [BUG] > Currently btrfs can create subvolume with an invalid qgroup inherit > without triggering any error: > > # mkfs.btrfs -O quota -f $dev > # mount $dev $mnt > # btrfs subvolume create -i 2/0 $mnt/subv1 > # btrfs qgroup show -prce --sync $mnt > Qgroupid Referenced Exclusive Path > -------- ---------- --------- ---- > 0/5 16.00KiB 16.00KiB <toplevel> > 0/256 16.00KiB 16.00KiB subv1 > > [CAUSE] > We only do a very basic size check for btrfs_qgroup_inherit structure, > but never really verify if the values are correct. > > Thus in btrfs_qgroup_inherit() function, we have to skip non-existing > qgroups, and never return any error. > > [FIX] > Fix the behavior and introduce extra checks: > > - Introduce early check for btrfs_qgroup_inherit structure > Not only the size, but also all the qgroup ids would be verifyed. > > And the timing is very early, so we can return error early. > This early check is very important for snapshot creation, as snapshot > is delayed to transaction commit. > > - Drop support for btrfs_qgroup_inherit::num_ref_copies and > num_excl_copies > Those two members are used to specify to copy refr/excl numbers from > other qgroups. > This would definitely mark qgroup inconsistent, and btrfs-progs has > dropped the support for them for a long time. > It's time to drop the support for kernel. > > - Verify the supported btrfs_qgroup_inherit::flags > Just in case we want to add extra flags for btrfs_qgroup_inherit. > > Now above subvolume creation would fail with -ENOENT other than silently > ignore the non-existing qgroup. > > CC: stable@vger.kernel.org > Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: David Sterba <dsterba@suse.com>
在 2024/3/1 22:31, David Sterba 写道: > On Tue, Feb 27, 2024 at 01:45:35PM +1030, Qu Wenruo wrote: >> [BUG] >> Currently btrfs can create subvolume with an invalid qgroup inherit >> without triggering any error: >> >> # mkfs.btrfs -O quota -f $dev >> # mount $dev $mnt >> # btrfs subvolume create -i 2/0 $mnt/subv1 >> # btrfs qgroup show -prce --sync $mnt >> Qgroupid Referenced Exclusive Path >> -------- ---------- --------- ---- >> 0/5 16.00KiB 16.00KiB <toplevel> >> 0/256 16.00KiB 16.00KiB subv1 >> >> [CAUSE] >> We only do a very basic size check for btrfs_qgroup_inherit structure, >> but never really verify if the values are correct. >> >> Thus in btrfs_qgroup_inherit() function, we have to skip non-existing >> qgroups, and never return any error. >> >> [FIX] >> Fix the behavior and introduce extra checks: >> >> - Introduce early check for btrfs_qgroup_inherit structure >> Not only the size, but also all the qgroup ids would be verifyed. >> >> And the timing is very early, so we can return error early. >> This early check is very important for snapshot creation, as snapshot >> is delayed to transaction commit. >> >> - Drop support for btrfs_qgroup_inherit::num_ref_copies and >> num_excl_copies >> Those two members are used to specify to copy refr/excl numbers from >> other qgroups. >> This would definitely mark qgroup inconsistent, and btrfs-progs has >> dropped the support for them for a long time. >> It's time to drop the support for kernel. >> >> - Verify the supported btrfs_qgroup_inherit::flags >> Just in case we want to add extra flags for btrfs_qgroup_inherit. >> >> Now above subvolume creation would fail with -ENOENT other than silently >> ignore the non-existing qgroup. >> >> CC: stable@vger.kernel.org >> Signed-off-by: Qu Wenruo <wqu@suse.com> > > Reviewed-by: David Sterba <dsterba@suse.com> > Just one thing to notice, this would cause certain test cases to fail, as previously any incorrect qgroup inherit would just be ignored, but now it would error out explicitly. IIRC there are 2 or 3 failures, one already fixed. I'll send out the remaining fixes for fstests. Thanks, Qu
On Sat, Mar 02, 2024 at 06:51:58AM +1030, Qu Wenruo wrote: > 在 2024/3/1 22:31, David Sterba 写道: > > On Tue, Feb 27, 2024 at 01:45:35PM +1030, Qu Wenruo wrote: > >> [BUG] > >> Currently btrfs can create subvolume with an invalid qgroup inherit > >> without triggering any error: > >> > >> # mkfs.btrfs -O quota -f $dev > >> # mount $dev $mnt > >> # btrfs subvolume create -i 2/0 $mnt/subv1 > >> # btrfs qgroup show -prce --sync $mnt > >> Qgroupid Referenced Exclusive Path > >> -------- ---------- --------- ---- > >> 0/5 16.00KiB 16.00KiB <toplevel> > >> 0/256 16.00KiB 16.00KiB subv1 > >> > >> [CAUSE] > >> We only do a very basic size check for btrfs_qgroup_inherit structure, > >> but never really verify if the values are correct. > >> > >> Thus in btrfs_qgroup_inherit() function, we have to skip non-existing > >> qgroups, and never return any error. > >> > >> [FIX] > >> Fix the behavior and introduce extra checks: > >> > >> - Introduce early check for btrfs_qgroup_inherit structure > >> Not only the size, but also all the qgroup ids would be verifyed. > >> > >> And the timing is very early, so we can return error early. > >> This early check is very important for snapshot creation, as snapshot > >> is delayed to transaction commit. > >> > >> - Drop support for btrfs_qgroup_inherit::num_ref_copies and > >> num_excl_copies > >> Those two members are used to specify to copy refr/excl numbers from > >> other qgroups. > >> This would definitely mark qgroup inconsistent, and btrfs-progs has > >> dropped the support for them for a long time. > >> It's time to drop the support for kernel. > >> > >> - Verify the supported btrfs_qgroup_inherit::flags > >> Just in case we want to add extra flags for btrfs_qgroup_inherit. > >> > >> Now above subvolume creation would fail with -ENOENT other than silently > >> ignore the non-existing qgroup. > >> > >> CC: stable@vger.kernel.org > >> Signed-off-by: Qu Wenruo <wqu@suse.com> > > > > Reviewed-by: David Sterba <dsterba@suse.com> > > > Just one thing to notice, this would cause certain test cases to fail, > as previously any incorrect qgroup inherit would just be ignored, but > now it would error out explicitly. Ok, this is expected if we do fixes like that.
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 8b80fbea1e72..c19ce2e292dc 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -1382,7 +1382,7 @@ static noinline int btrfs_ioctl_snap_create_v2(struct file *file, if (vol_args->flags & BTRFS_SUBVOL_RDONLY) readonly = true; if (vol_args->flags & BTRFS_SUBVOL_QGROUP_INHERIT) { - u64 nums; + struct btrfs_fs_info *fs_info = inode_to_fs_info(file_inode(file)); if (vol_args->size < sizeof(*inherit) || vol_args->size > PAGE_SIZE) { @@ -1395,19 +1395,9 @@ static noinline int btrfs_ioctl_snap_create_v2(struct file *file, goto free_args; } - if (inherit->num_qgroups > PAGE_SIZE || - inherit->num_ref_copies > PAGE_SIZE || - inherit->num_excl_copies > PAGE_SIZE) { - ret = -EINVAL; + ret = btrfs_qgroup_check_inherit(fs_info, inherit, vol_args->size); + if (ret < 0) goto free_inherit; - } - - nums = inherit->num_qgroups + 2 * inherit->num_ref_copies + - 2 * inherit->num_excl_copies; - if (vol_args->size != struct_size(inherit, qgroups, nums)) { - ret = -EINVAL; - goto free_inherit; - } } ret = __btrfs_ioctl_snap_create(file, file_mnt_idmap(file), diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c index 4fa83c76b37b..66968092b554 100644 --- a/fs/btrfs/qgroup.c +++ b/fs/btrfs/qgroup.c @@ -3046,6 +3046,58 @@ int btrfs_run_qgroups(struct btrfs_trans_handle *trans) return ret; } +int btrfs_qgroup_check_inherit(struct btrfs_fs_info *fs_info, + struct btrfs_qgroup_inherit *inherit, + size_t size) +{ + if (inherit->flags & ~BTRFS_QGROUP_INHERIT_FLAGS_SUPP) + return -EOPNOTSUPP; + if (size < sizeof(*inherit) || size > PAGE_SIZE) + return -EINVAL; + + /* + * In the past we allow btrfs_qgroup_inherit to specify to copy + * refr/excl numbers directly from other qgroups. + * This behavior has been disable in btrfs-progs for a very long time, + * but here we should also disable them for kernel, as this behavior + * is known to mark qgroup inconsistent, and a rescan would wipe out the + * change anyway. + * + * So here we just reject any btrfs_qgroup_inherit with num_ref_copies or + * num_excl_copies. + */ + if (inherit->num_ref_copies || inherit->num_excl_copies) + return -EINVAL; + + if (inherit->num_qgroups > PAGE_SIZE) + return -EINVAL; + + if (size != struct_size(inherit, qgroups, inherit->num_qgroups)) + return -EINVAL; + + /* + * Now check all the remaining qgroups, they should all: + * - Exist + * - Be higher level qgroups. + */ + for (int i = 0; i < inherit->num_qgroups; i++) { + struct btrfs_qgroup *qgroup; + u64 qgroupid = inherit->qgroups[i]; + + if (btrfs_qgroup_level(qgroupid) == 0) + return -EINVAL; + + spin_lock(&fs_info->qgroup_lock); + qgroup = find_qgroup_rb(fs_info, qgroupid); + if (!qgroup) { + spin_unlock(&fs_info->qgroup_lock); + return -ENOENT; + } + spin_unlock(&fs_info->qgroup_lock); + } + return 0; +} + static int qgroup_auto_inherit(struct btrfs_fs_info *fs_info, u64 inode_rootid, struct btrfs_qgroup_inherit **inherit) diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h index 1f664261c064..706640be0ec2 100644 --- a/fs/btrfs/qgroup.h +++ b/fs/btrfs/qgroup.h @@ -350,6 +350,9 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr, struct ulist *new_roots); int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans); int btrfs_run_qgroups(struct btrfs_trans_handle *trans); +int btrfs_qgroup_check_inherit(struct btrfs_fs_info *fs_info, + struct btrfs_qgroup_inherit *inherit, + size_t size); int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid, u64 objectid, u64 inode_rootid, struct btrfs_qgroup_inherit *inherit); diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h index f8bc34a6bcfa..cdf6ad872149 100644 --- a/include/uapi/linux/btrfs.h +++ b/include/uapi/linux/btrfs.h @@ -92,6 +92,7 @@ struct btrfs_qgroup_limit { * struct btrfs_qgroup_inherit.flags */ #define BTRFS_QGROUP_INHERIT_SET_LIMITS (1ULL << 0) +#define BTRFS_QGROUP_INHERIT_FLAGS_SUPP (BTRFS_QGROUP_INHERIT_SET_LIMITS) struct btrfs_qgroup_inherit { __u64 flags;
[BUG] Currently btrfs can create subvolume with an invalid qgroup inherit without triggering any error: # mkfs.btrfs -O quota -f $dev # mount $dev $mnt # btrfs subvolume create -i 2/0 $mnt/subv1 # btrfs qgroup show -prce --sync $mnt Qgroupid Referenced Exclusive Path -------- ---------- --------- ---- 0/5 16.00KiB 16.00KiB <toplevel> 0/256 16.00KiB 16.00KiB subv1 [CAUSE] We only do a very basic size check for btrfs_qgroup_inherit structure, but never really verify if the values are correct. Thus in btrfs_qgroup_inherit() function, we have to skip non-existing qgroups, and never return any error. [FIX] Fix the behavior and introduce extra checks: - Introduce early check for btrfs_qgroup_inherit structure Not only the size, but also all the qgroup ids would be verifyed. And the timing is very early, so we can return error early. This early check is very important for snapshot creation, as snapshot is delayed to transaction commit. - Drop support for btrfs_qgroup_inherit::num_ref_copies and num_excl_copies Those two members are used to specify to copy refr/excl numbers from other qgroups. This would definitely mark qgroup inconsistent, and btrfs-progs has dropped the support for them for a long time. It's time to drop the support for kernel. - Verify the supported btrfs_qgroup_inherit::flags Just in case we want to add extra flags for btrfs_qgroup_inherit. Now above subvolume creation would fail with -ENOENT other than silently ignore the non-existing qgroup. CC: stable@vger.kernel.org Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/ioctl.c | 16 +++--------- fs/btrfs/qgroup.c | 52 ++++++++++++++++++++++++++++++++++++++ fs/btrfs/qgroup.h | 3 +++ include/uapi/linux/btrfs.h | 1 + 4 files changed, 59 insertions(+), 13 deletions(-)