diff mbox series

[v3] btrfs: free device without BTRFS_MAGIC

Message ID e15a2f44a540c1e036e19664fd3a8220045dd762.1601470709.git.anand.jain@oracle.com
State New, archived
Headers show
Series [v3] btrfs: free device without BTRFS_MAGIC | expand

Commit Message

Anand Jain Sept. 30, 2020, 1:09 p.m. UTC
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(-)

Comments

Anand Jain Oct. 1, 2020, 1:05 a.m. UTC | #1
David,

  FYI this patch is the fix for fstests btrfs/198

Thanks, Anand
David Sterba Oct. 1, 2020, 10:49 a.m. UTC | #2
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 mbox series

Patch

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;