| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 10 Aug 2022 15:47:53 +0800
From: Qu Wenruo <quwenruo.btrfs@....com>
To: "Flint.Wang" <hmsjwzb@...o.com>
Cc: anand.jain@...cle.com, nborisov@...e.com, strongbox8@...o.com,
Chris Mason <clm@...com>, Josef Bacik <josef@...icpanda.com>,
David Sterba <dsterba@...e.com>, linux-btrfs@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] btrfs: Fix btrfs_find_device for btrfs/249
On 2022/8/10 15:20, Flint.Wang wrote:
> testcase btrfs249 failed.
> [How to reproduce]
> mkfs.btrfs -f -d raid1 -m raid1 /dev/sdb /dev/sdc
> btrfstune -S 1 /dev/sdb
> wipefs -a /dev/sdb
> mount -o degraded /dev/sdc /mnt/scratch
> btrfs device add -f /dev/sdd /mnt/scratch
> btrfs filesystem usage /mnt/scratch
>
> [Root cause]
> mkfs.btrfs -f -d raid1 -m raid1 /dev/sdb /dev/sdc
> btrfstune -S 1 /dev/sdb
> wipefs -a /dev/sdb
> mount -o degraded /dev/sdc /mnt/scratch
>
> In the above commands, btrfstune command set the sdb and sdc to seeding device.
> After that you wipe the filesystem on sdb. After mount, you will find the status of sdb is missing.
>
> btrfs device add -f /dev/sdd /mnt/scratch:
> This command will invoke btrfs_setup_sprout to do the job.
> It put the devices on fs_devices->devices to seed_devices list.
> So only sdd is on the fs_devices->devices list. sdb(missing), sdc on the seed_devices list.
> But when we look into the btrfs_find_devices function, it find devices both in devices list and seed_devices list.
>
> btrfs filesystem usage /mnt/scratch
> This command use ioctl to get device info. The assertion is triggered because it finds the number of devices is inconsistent.
>
> [My fix solution]
> 1. Add noseed argument to btrfs_find_device. It force the function only look into devices list.
You can put the new @noseed argument into btrfs_dev_lookup_args, then no
need to change all callers.
> 2. Add a new ioctl request(BTRFS_IOC_DEV_INFO_NOSEED) in case some application may depend the original ioctl behavior on BTRFS_IOC_DEV_INFO
This is too heavy just for this particular case, I stronglly doubt if we
really need to go this way.
> 3. Modify load_device_info and load_chunk_and_device_info in btrfs-prog for appropriate ioctl call.
To me, we can still handle the work just inside btrfs-progs, thus no
need to introducing a completely new ioctl which has the same functionality.
E.g, go tree-search ioctl to search chunk tree to get a correct number
of devices, instead of just relying on fi_args->num_devices.
A fix in btrfs-progs is much simpler and easier to maintain than a full
new ioctl.
Thanks,
Qu
>
> After the change, btrfs249 passed.
>
> Signed-off-by: Flint.Wang <hmsjwzb@...o.com>
> ---
> fs/btrfs/dev-replace.c | 8 ++++----
> fs/btrfs/ioctl.c | 10 ++++++----
> fs/btrfs/scrub.c | 4 ++--
> fs/btrfs/volumes.c | 22 ++++++++++++----------
> fs/btrfs/volumes.h | 5 ++++-
> include/uapi/linux/btrfs.h | 2 ++
> 6 files changed, 30 insertions(+), 21 deletions(-)
>
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index f43196a893ca3..49d3c587c2948 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -101,7 +101,7 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
> * We don't have a replace item or it's corrupted. If there is
> * a replace target, fail the mount.
> */
> - if (btrfs_find_device(fs_info->fs_devices, &args)) {
> + if (btrfs_find_device(fs_info->fs_devices, &args, false)) {
> btrfs_err(fs_info,
> "found replace target device without a valid replace item");
> ret = -EUCLEAN;
> @@ -163,7 +163,7 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
> * We don't have an active replace item but if there is a
> * replace target, fail the mount.
> */
> - if (btrfs_find_device(fs_info->fs_devices, &args)) {
> + if (btrfs_find_device(fs_info->fs_devices, &args, false)) {
> btrfs_err(fs_info,
> "replace devid present without an active replace item");
> ret = -EUCLEAN;
> @@ -174,9 +174,9 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
> break;
> case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
> case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
> - dev_replace->tgtdev = btrfs_find_device(fs_info->fs_devices, &args);
> + dev_replace->tgtdev = btrfs_find_device(fs_info->fs_devices, &args, false);
> args.devid = src_devid;
> - dev_replace->srcdev = btrfs_find_device(fs_info->fs_devices, &args);
> + dev_replace->srcdev = btrfs_find_device(fs_info->fs_devices, &args, false);
>
> /*
> * allow 'btrfs dev replace_cancel' if src/tgt device is
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index fe0cc816b4eba..bdf1578839c99 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2039,7 +2039,7 @@ static noinline int btrfs_ioctl_resize(struct file *file,
> }
>
> args.devid = devid;
> - device = btrfs_find_device(fs_info->fs_devices, &args);
> + device = btrfs_find_device(fs_info->fs_devices, &args, false);
> if (!device) {
> btrfs_info(fs_info, "resizer unable to find device %llu",
> devid);
> @@ -3721,7 +3721,7 @@ static long btrfs_ioctl_fs_info(struct btrfs_fs_info *fs_info,
> }
>
> static long btrfs_ioctl_dev_info(struct btrfs_fs_info *fs_info,
> - void __user *arg)
> + void __user *arg, bool noseed)
> {
> BTRFS_DEV_LOOKUP_ARGS(args);
> struct btrfs_ioctl_dev_info_args *di_args;
> @@ -3737,7 +3737,7 @@ static long btrfs_ioctl_dev_info(struct btrfs_fs_info *fs_info,
> args.uuid = di_args->uuid;
>
> rcu_read_lock();
> - dev = btrfs_find_device(fs_info->fs_devices, &args);
> + dev = btrfs_find_device(fs_info->fs_devices, &args, noseed);
> if (!dev) {
> ret = -ENODEV;
> goto out;
> @@ -5468,7 +5468,7 @@ long btrfs_ioctl(struct file *file, unsigned int
> case BTRFS_IOC_FS_INFO:
> return btrfs_ioctl_fs_info(fs_info, argp);
> case BTRFS_IOC_DEV_INFO:
> - return btrfs_ioctl_dev_info(fs_info, argp);
> + return btrfs_ioctl_dev_info(fs_info, argp, false);
> case BTRFS_IOC_TREE_SEARCH:
> return btrfs_ioctl_tree_search(inode, argp);
> case BTRFS_IOC_TREE_SEARCH_V2:
> @@ -5570,6 +5570,8 @@ long btrfs_ioctl(struct file *file, unsigned int
> case BTRFS_IOC_ENCODED_WRITE_32:
> return btrfs_ioctl_encoded_write(file, argp, true);
> #endif
> + case BTRFS_IOC_DEV_INFO_NOSEED:
> + return btrfs_ioctl_dev_info(fs_info, argp, true);
> }
>
> return -ENOTTY;
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index 3afe5fa50a631..4b734d76776ca 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -4143,7 +4143,7 @@ int btrfs_scrub_dev(struct btrfs_fs_info *fs_info, u64 devid, u64 start,
> goto out_free_ctx;
>
> mutex_lock(&fs_info->fs_devices->device_list_mutex);
> - dev = btrfs_find_device(fs_info->fs_devices, &args);
> + dev = btrfs_find_device(fs_info->fs_devices, &args, false);
> if (!dev || (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state) &&
> !is_dev_replace)) {
> mutex_unlock(&fs_info->fs_devices->device_list_mutex);
> @@ -4321,7 +4321,7 @@ int btrfs_scrub_progress(struct btrfs_fs_info *fs_info, u64 devid,
> struct scrub_ctx *sctx = NULL;
>
> mutex_lock(&fs_info->fs_devices->device_list_mutex);
> - dev = btrfs_find_device(fs_info->fs_devices, &args);
> + dev = btrfs_find_device(fs_info->fs_devices, &args, false);
> if (dev)
> sctx = dev->scrub_ctx;
> if (sctx)
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 272901514b0c1..1abd75e90cd9e 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -808,7 +808,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
> };
>
> mutex_lock(&fs_devices->device_list_mutex);
> - device = btrfs_find_device(fs_devices, &args);
> + device = btrfs_find_device(fs_devices, &args, false);
>
> /*
> * If this disk has been pulled into an fs devices created by
> @@ -2075,7 +2075,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info,
> if (ret)
> return ret;
>
> - device = btrfs_find_device(fs_info->fs_devices, args);
> + device = btrfs_find_device(fs_info->fs_devices, args, false);
> if (!device) {
> if (args->missing)
> ret = BTRFS_ERROR_DEV_MISSING_NOT_FOUND;
> @@ -2381,7 +2381,7 @@ struct btrfs_device *btrfs_find_device_by_devspec(
>
> if (devid) {
> args.devid = devid;
> - device = btrfs_find_device(fs_info->fs_devices, &args);
> + device = btrfs_find_device(fs_info->fs_devices, &args, false);
> if (!device)
> return ERR_PTR(-ENOENT);
> return device;
> @@ -2390,7 +2390,7 @@ struct btrfs_device *btrfs_find_device_by_devspec(
> ret = btrfs_get_dev_args_from_path(fs_info, &args, device_path);
> if (ret)
> return ERR_PTR(ret);
> - device = btrfs_find_device(fs_info->fs_devices, &args);
> + device = btrfs_find_device(fs_info->fs_devices, &args, false);
> btrfs_put_dev_args_from_path(&args);
> if (!device)
> return ERR_PTR(-ENOENT);
> @@ -2551,7 +2551,7 @@ static int btrfs_finish_sprout(struct btrfs_trans_handle *trans)
> BTRFS_FSID_SIZE);
> args.uuid = dev_uuid;
> args.fsid = fs_uuid;
> - device = btrfs_find_device(fs_info->fs_devices, &args);
> + device = btrfs_find_device(fs_info->fs_devices, &args, false);
> BUG_ON(!device); /* Logic error */
>
> if (device->fs_devices->seeding) {
> @@ -6821,7 +6821,7 @@ static bool dev_args_match_device(const struct btrfs_dev_lookup_args *args,
> * only devid is used.
> */
> struct btrfs_device *btrfs_find_device(const struct btrfs_fs_devices *fs_devices,
> - const struct btrfs_dev_lookup_args *args)
> + const struct btrfs_dev_lookup_args *args, bool noseed)
> {
> struct btrfs_device *device;
> struct btrfs_fs_devices *seed_devs;
> @@ -6832,6 +6832,8 @@ struct btrfs_device *btrfs_find_device(const struct btrfs_fs_devices *fs_devices
> return device;
> }
> }
> + if (noseed)
> + return NULL;
>
> list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) {
> if (!dev_args_match_fs_devices(args, seed_devs))
> @@ -7095,7 +7097,7 @@ static int read_one_chunk(struct btrfs_key *key, struct extent_buffer *leaf,
> btrfs_stripe_dev_uuid_nr(chunk, i),
> BTRFS_UUID_SIZE);
> args.uuid = uuid;
> - map->stripes[i].dev = btrfs_find_device(fs_info->fs_devices, &args);
> + map->stripes[i].dev = btrfs_find_device(fs_info->fs_devices, &args, false);
> if (!map->stripes[i].dev) {
> map->stripes[i].dev = handle_missing_device(fs_info,
> devid, uuid);
> @@ -7226,7 +7228,7 @@ static int read_one_dev(struct extent_buffer *leaf,
> return PTR_ERR(fs_devices);
> }
>
> - device = btrfs_find_device(fs_info->fs_devices, &args);
> + device = btrfs_find_device(fs_info->fs_devices, &args, false);
> if (!device) {
> if (!btrfs_test_opt(fs_info, DEGRADED)) {
> btrfs_report_missing_device(fs_info, devid,
> @@ -7884,7 +7886,7 @@ int btrfs_get_dev_stats(struct btrfs_fs_info *fs_info,
>
> mutex_lock(&fs_devices->device_list_mutex);
> args.devid = stats->devid;
> - dev = btrfs_find_device(fs_info->fs_devices, &args);
> + dev = btrfs_find_device(fs_info->fs_devices, &args, false);
> mutex_unlock(&fs_devices->device_list_mutex);
>
> if (!dev) {
> @@ -8026,7 +8028,7 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
> }
>
> /* Make sure no dev extent is beyond device boundary */
> - dev = btrfs_find_device(fs_info->fs_devices, &args);
> + dev = btrfs_find_device(fs_info->fs_devices, &args, false);
> if (!dev) {
> btrfs_err(fs_info, "failed to find devid %llu", devid);
> ret = -EUCLEAN;
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 5639961b3626f..4b6bcc777f752 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -609,7 +609,10 @@ int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len);
> int btrfs_grow_device(struct btrfs_trans_handle *trans,
> struct btrfs_device *device, u64 new_size);
> struct btrfs_device *btrfs_find_device(const struct btrfs_fs_devices *fs_devices,
> - const struct btrfs_dev_lookup_args *args);
> + const struct btrfs_dev_lookup_args *args,
> + bool noseed);
> +struct btrfs_device *btrfs_find_device_noseed(const struct btrfs_fs_devices *fs_devices,
> + const struct btrfs_dev_lookup_args *args);
> int btrfs_shrink_device(struct btrfs_device *device, u64 new_size);
> int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *path);
> int btrfs_balance(struct btrfs_fs_info *fs_info,
> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
> index 7ada84e4a3ed1..880b565479a12 100644
> --- a/include/uapi/linux/btrfs.h
> +++ b/include/uapi/linux/btrfs.h
> @@ -1078,6 +1078,8 @@ enum btrfs_err_code {
> struct btrfs_ioctl_scrub_args)
> #define BTRFS_IOC_DEV_INFO _IOWR(BTRFS_IOCTL_MAGIC, 30, \
> struct btrfs_ioctl_dev_info_args)
> +#define BTRFS_IOC_DEV_INFO_NOSEED _IOR(BTRFS_IOCTL_MAGIC, 30, \
> + struct btrfs_ioctl_dev_info_args)
> #define BTRFS_IOC_FS_INFO _IOR(BTRFS_IOCTL_MAGIC, 31, \
> struct btrfs_ioctl_fs_info_args)
> #define BTRFS_IOC_BALANCE_V2 _IOWR(BTRFS_IOCTL_MAGIC, 32, \
Powered by blists - more mailing lists