| Message ID | e15a2f44a540c1e036e19664fd3a8220045dd762.1601470709.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] btrfs: free device without BTRFS_MAGIC | expand |
David, FYI this patch is the fix for fstests btrfs/198 Thanks, Anand
On Wed, Sep 30, 2020 at 09:09:52PM +0800, Anand Jain wrote: > Many things can happen after the device is scanned and before the device > is mounted. > > One such thing is losing the BTRFS_MAGIC on the device. > > If it happens we still won't free that device from the memory and causes > the userland to confuse. > > For example: As the BTRFS_IOC_DEV_INFO still carries the device path which > does not have the BTRFS_MAGIC, the btrfs fi show still shows device > which does not belong. As shown below. > > mkfs.btrfs -fq -draid1 -mraid1 /dev/sda /dev/sdb > > wipefs -a /dev/sdb > mount -o degraded /dev/sda /btrfs > btrfs fi show -m > > /dev/sdb does not contain BTRFS_MAGIC and we still show it as part of > btrfs. > Label: none uuid: 470ec6fb-646b-4464-b3cb-df1b26c527bd > Total devices 2 FS bytes used 128.00KiB > devid 1 size 3.00GiB used 571.19MiB path /dev/sda > devid 2 size 3.00GiB used 571.19MiB path /dev/sdb > > Fix is to return -ENODATA error code in btrfs_read_dev_one_super() > when BTRFS_MAGIC check fails, and its parent open_fs_devices() to > free the device in the mount-thread. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > Reviewed-by: Josef Bacik <josef@toxicpanda.com> > --- > v3: First check for the BTRFS_MAGIC and then the bytenr check. > Add rb. > v2: Do not return ENODATA on `btrfs_super_bytenr(super) != bytenr` > > fs/btrfs/disk-io.c | 8 ++++++-- > fs/btrfs/volumes.c | 19 +++++++++++++------ > 2 files changed, 19 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 3d39f5d47ad3..764001609a15 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -3424,8 +3424,12 @@ struct btrfs_super_block *btrfs_read_dev_one_super(struct block_device *bdev, > return ERR_CAST(page); > > super = page_address(page); > - if (btrfs_super_bytenr(super) != bytenr || > - btrfs_super_magic(super) != BTRFS_MAGIC) { > + if (btrfs_super_magic(super) != BTRFS_MAGIC) { > + btrfs_release_disk_super(super); > + return ERR_PTR(-ENODATA); > + } > + > + if (btrfs_super_bytenr(super) != bytenr) { > btrfs_release_disk_super(super); > return ERR_PTR(-EINVAL); > } > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index c69da5eb7636..a208043b4419 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -1197,17 +1197,24 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices, > { > struct btrfs_device *device; > struct btrfs_device *latest_dev = NULL; > + struct btrfs_device *tmp_device; > > flags |= FMODE_EXCL; > > - list_for_each_entry(device, &fs_devices->devices, dev_list) { > - /* Just open everything we can; ignore failures here */ > - if (btrfs_open_one_device(fs_devices, device, flags, holder)) > - continue; > + list_for_each_entry_safe(device, tmp_device, &fs_devices->devices, > + dev_list) { > + int ret; > > - if (!latest_dev || > - device->generation > latest_dev->generation) > + /* Just open everything we can; ignore failures here */ The comment does not make sense anymore, the failures are checked now. > + ret = btrfs_open_one_device(fs_devices, device, flags, holder); > + if (ret == 0 && (!latest_dev || > + device->generation > latest_dev->generation)) { > latest_dev = device; > + } else if (ret == -ENODATA) { > + fs_devices->num_devices--; > + list_del(&device->dev_list); > + btrfs_free_device(device); Now ret is ENODATA and we don't want to propagate that up the to the callers. It's not a problem right now as open_fs_devices returns directly or 0 at the end. Otherwise it looks ok to me.
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 3d39f5d47ad3..764001609a15 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -3424,8 +3424,12 @@ struct btrfs_super_block *btrfs_read_dev_one_super(struct block_device *bdev, return ERR_CAST(page); super = page_address(page); - if (btrfs_super_bytenr(super) != bytenr || - btrfs_super_magic(super) != BTRFS_MAGIC) { + if (btrfs_super_magic(super) != BTRFS_MAGIC) { + btrfs_release_disk_super(super); + return ERR_PTR(-ENODATA); + } + + if (btrfs_super_bytenr(super) != bytenr) { btrfs_release_disk_super(super); return ERR_PTR(-EINVAL); } diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index c69da5eb7636..a208043b4419 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -1197,17 +1197,24 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices, { struct btrfs_device *device; struct btrfs_device *latest_dev = NULL; + struct btrfs_device *tmp_device; flags |= FMODE_EXCL; - list_for_each_entry(device, &fs_devices->devices, dev_list) { - /* Just open everything we can; ignore failures here */ - if (btrfs_open_one_device(fs_devices, device, flags, holder)) - continue; + list_for_each_entry_safe(device, tmp_device, &fs_devices->devices, + dev_list) { + int ret; - if (!latest_dev || - device->generation > latest_dev->generation) + /* Just open everything we can; ignore failures here */ + ret = btrfs_open_one_device(fs_devices, device, flags, holder); + if (ret == 0 && (!latest_dev || + device->generation > latest_dev->generation)) { latest_dev = device; + } else if (ret == -ENODATA) { + fs_devices->num_devices--; + list_del(&device->dev_list); + btrfs_free_device(device); + } } if (fs_devices->open_devices == 0) return -EINVAL;
Many things can happen after the device is scanned and before the device is mounted. One such thing is losing the BTRFS_MAGIC on the device. If it happens we still won't free that device from the memory and causes the userland to confuse. For example: As the BTRFS_IOC_DEV_INFO still carries the device path which does not have the BTRFS_MAGIC, the btrfs fi show still shows device which does not belong. As shown below. mkfs.btrfs -fq -draid1 -mraid1 /dev/sda /dev/sdb wipefs -a /dev/sdb mount -o degraded /dev/sda /btrfs btrfs fi show -m /dev/sdb does not contain BTRFS_MAGIC and we still show it as part of btrfs. Label: none uuid: 470ec6fb-646b-4464-b3cb-df1b26c527bd Total devices 2 FS bytes used 128.00KiB devid 1 size 3.00GiB used 571.19MiB path /dev/sda devid 2 size 3.00GiB used 571.19MiB path /dev/sdb Fix is to return -ENODATA error code in btrfs_read_dev_one_super() when BTRFS_MAGIC check fails, and its parent open_fs_devices() to free the device in the mount-thread. Signed-off-by: Anand Jain <anand.jain@oracle.com> Reviewed-by: Josef Bacik <josef@toxicpanda.com> --- v3: First check for the BTRFS_MAGIC and then the bytenr check. Add rb. v2: Do not return ENODATA on `btrfs_super_bytenr(super) != bytenr` fs/btrfs/disk-io.c | 8 ++++++-- fs/btrfs/volumes.c | 19 +++++++++++++------ 2 files changed, 19 insertions(+), 8 deletions(-)