[PATCH RESEND v2 0/3] cleanup btrfs_find_device and fix sysbot warning

[PATCH RESEND v2 0/3] cleanup btrfs_find_device and fix sysbot warning

[Anand Jain]

Patch 1 and 2 are cleanups. They were sent before with the cover-letter
as below.
 [PATCH RESEND 0/2] fix verify_one_dev_extent and btrfs_find_device

Patch 3 is a fix for the Warning reported by sysbot. This also was sent
before with the cover-letter titled as below.
  [PATCH 0/1] handle attacking device with devid=0

To avoid conflict fixes, please apply these patches in the order it is
sent here. But the sets are not related.

Both of these patchsets are at v2 at the latest. So I am marking all of
them as v2. The individual changes are in the patches.

Also earlier resend patches were threaded under its previous cover-letter
(though --in-reply-to was not used to generate/send those patches). So I am
trying to resend these patches again, hopefully, they are not threaded to
the older series again.

Anand Jain (3):
  btrfs: drop never met condition of disk_total_bytes == 0
  btrfs: fix btrfs_find_device unused arg seed
  btrfs: fix devid 0 without a replace item by failing the mount

 fs/btrfs/dev-replace.c | 30 ++++++++++++++---
 fs/btrfs/ioctl.c       |  4 +--
 fs/btrfs/scrub.c       |  4 +--
 fs/btrfs/volumes.c     | 73 ++++++++++++++++--------------------------
 fs/btrfs/volumes.h     |  2 +-
 5 files changed, 58 insertions(+), 55 deletions(-)

-- 
2.25.1

[PATCH RESEND v2 1/3] btrfs: drop never met condition of disk_total_bytes == 0

[Anand Jain]

btrfs_device::disk_total_bytes is set even for a seed device (the
comment is wrong).

The function fill_device_from_item() does the job of reading it from the
item and updating btrfs_device::disk_total_bytes. So both the missing
device and the seed devices do have their disk_total_bytes updated.

Furthermore, while removing the device if there is a power loss, we could
have a device with its total_bytes = 0, that's still valid.

So this patch removes the check dev->disk_total_bytes == 0 in the
function verify_one_dev_extent(), which it does nothing in it.

And take this opportunity to introduce a check if the device::total_bytes
is more than the max device size in read_one_dev().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
v2: add check if the total_bytes is more than the actual device size in
    read_one_dev().
    update change log.

 fs/btrfs/volumes.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index eb9ee7c2998f..e615ca393aab 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6875,6 +6875,16 @@ static int read_one_dev(struct extent_buffer *leaf,
 	}
 
 	fill_device_from_item(leaf, dev_item, device);
+	if (device->bdev) {
+		u64 max_total_bytes = i_size_read(device->bdev->bd_inode);
+
+		if (device->total_bytes > max_total_bytes) {
+			btrfs_err(fs_info,
+			"device total_bytes should be below %llu but found %llu",
+				  max_total_bytes, device->total_bytes);
+			return -EINVAL;
+		}
+	}
 	set_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &device->dev_state);
 	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
 	   !test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state)) {
@@ -7608,21 +7618,6 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
 		goto out;
 	}
 
-	/* It's possible this device is a dummy for seed device */
-	if (dev->disk_total_bytes == 0) {
-		struct btrfs_fs_devices *devs;
-
-		devs = list_first_entry(&fs_info->fs_devices->seed_list,
-					struct btrfs_fs_devices, seed_list);
-		dev = btrfs_find_device(devs, devid, NULL, NULL, false);
-		if (!dev) {
-			btrfs_err(fs_info, "failed to find seed devid %llu",
-				  devid);
-			ret = -EUCLEAN;
-			goto out;
-		}
-	}
-
 	if (physical_offset + physical_len > dev->disk_total_bytes) {
 		btrfs_err(fs_info,
 "dev extent devid %llu physical offset %llu len %llu is beyond device boundary %llu",
-- 
2.25.1

Re: [PATCH RESEND v2 1/3] btrfs: drop never met condition of disk_total_bytes == 0

[David Sterba]

On Tue, Nov 03, 2020 at 01:49:42PM +0800, Anand Jain wrote:
> btrfs_device::disk_total_bytes is set even for a seed device (the
> comment is wrong).

That's where I'm a bit lost. It was added in 1b3922a8bc74 ("btrfs: Use
real device structure to verify dev extent").

> The function fill_device_from_item() does the job of reading it from the
> item and updating btrfs_device::disk_total_bytes. So both the missing
> device and the seed devices do have their disk_total_bytes updated.
> 
> Furthermore, while removing the device if there is a power loss, we could
> have a device with its total_bytes = 0, that's still valid.

Ok, that's the condition that the commit mentioned above used to detect
the device and to avoid doing the tree-checker verification.

> So this patch removes the check dev->disk_total_bytes == 0 in the
> function verify_one_dev_extent(), which it does nothing in it.

Removing a check that supposedly does notghing, but the referenced
commit says otherwise.

> And take this opportunity to introduce a check if the device::total_bytes
> is more than the max device size in read_one_dev().

If this is not related to the the check removal, then it should be an
independent patch explaing in full what is being fixed. As I read it
this should be independent as it's checking the upper bound.

Re: [PATCH RESEND v2 1/3] btrfs: drop never met condition of disk_total_bytes == 0

[Anand Jain]

On 6/11/20 6:36 am, David Sterba wrote:
> On Tue, Nov 03, 2020 at 01:49:42PM +0800, Anand Jain wrote:
>> btrfs_device::disk_total_bytes is set even for a seed device (the
>> comment is wrong).
> 
> That's where I'm a bit lost. It was added in 1b3922a8bc74 ("btrfs: Use
> real device structure to verify dev extent").
> 

  The call chain where we update the btrfs_device::disk_total_bytes is as
  below..


    read_one_dev()
::

 From the section below we get the cloned copy of the seed device.
-----
         if (memcmp(fs_uuid, fs_devices->metadata_uuid, BTRFS_FSID_SIZE)) {
                 fs_devices = open_seed_devices(fs_info, fs_uuid);
                 if (IS_ERR(fs_devices))
                         return PTR_ERR(fs_devices);
         }

         device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
                                    fs_uuid);
------


  Further down in read_one_dev() at and in the 
fill_device_from_item(..., device) we update the 
btrfs_device::disk_total_bytes.

fill_device_from_item(..., device)
::
         device->disk_total_bytes = btrfs_device_total_bytes(leaf, 
dev_item);

Hope this clarifies.

V1 email discussion has more information.

>> The function fill_device_from_item() does the job of reading it from the
>> item and updating btrfs_device::disk_total_bytes. So both the missing
>> device and the seed devices do have their disk_total_bytes updated.
>>
>> Furthermore, while removing the device if there is a power loss, we could
>> have a device with its total_bytes = 0, that's still valid.


> 
> Ok, that's the condition that the commit mentioned above used to detect
> the device and to avoid doing the tree-checker verification.

Ok. Please look at what did the commit 1b3922a8bc74 do? It re-ran the 
btrfs_find_device(seed_devs,..., false), which anyway the 
btrfs_find_device(sprout_devs,..., true) has run just before. In both of 
these btrfs_find_device() runs, the dev returned will be the same. The 
fix makes no sense to the problem as in the commit.


> 
>> So this patch removes the check dev->disk_total_bytes == 0 in the
>> function verify_one_dev_extent(), which it does nothing in it.
> 
> Removing a check that supposedly does notghing, but the referenced
> commit says otherwise.
> 

IMO the reason for the problem found in that commit was wrong. Qu 
commit's email thread has some discussion. But nothing more on the problem.

Also Nikolay had the same question here was my reply.
https://www.spinics.net/lists/linux-btrfs/msg105645.html


>> And take this opportunity to introduce a check if the device::total_bytes
>> is more than the max device size in read_one_dev().
> 
> If this is not related to the the check removal, then it should be an
> independent patch explaing in full what is being fixed. As I read it
> this should be independent as it's checking the upper bound.
> 

  That came from the Josef comments, please refer to the v1 email 
discussion. Most of the above concerns are already discussed there. I am 
ok to move it to a new patch if required.

Thanks, Anand

Re: [PATCH RESEND v2 1/3] btrfs: drop never met condition of disk_total_bytes == 0

[David Sterba]

On Fri, Nov 06, 2020 at 08:20:12AM +0800, Anand Jain wrote:
> On 6/11/20 6:36 am, David Sterba wrote:
> > On Tue, Nov 03, 2020 at 01:49:42PM +0800, Anand Jain wrote:
> >> btrfs_device::disk_total_bytes is set even for a seed device (the
> >> comment is wrong).
> > 
> > That's where I'm a bit lost. It was added in 1b3922a8bc74 ("btrfs: Use
> > real device structure to verify dev extent").
> > 
> 
>   The call chain where we update the btrfs_device::disk_total_bytes is as
>   below..
> 
> 
>     read_one_dev()
> ::
> 
>  From the section below we get the cloned copy of the seed device.
> -----
>          if (memcmp(fs_uuid, fs_devices->metadata_uuid, BTRFS_FSID_SIZE)) {
>                  fs_devices = open_seed_devices(fs_info, fs_uuid);
>                  if (IS_ERR(fs_devices))
>                          return PTR_ERR(fs_devices);
>          }
> 
>          device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
>                                     fs_uuid);
> ------
> 
> 
>   Further down in read_one_dev() at and in the 
> fill_device_from_item(..., device) we update the 
> btrfs_device::disk_total_bytes.
> 
> fill_device_from_item(..., device)
> ::
>          device->disk_total_bytes = btrfs_device_total_bytes(leaf, 
> dev_item);
> 
> Hope this clarifies.
> 
> V1 email discussion has more information.

... that should have been turned into a changelog update in v2.

> >> The function fill_device_from_item() does the job of reading it from the
> >> item and updating btrfs_device::disk_total_bytes. So both the missing
> >> device and the seed devices do have their disk_total_bytes updated.
> >>
> >> Furthermore, while removing the device if there is a power loss, we could
> >> have a device with its total_bytes = 0, that's still valid.
> 
> 
> > 
> > Ok, that's the condition that the commit mentioned above used to detect
> > the device and to avoid doing the tree-checker verification.
> 
> Ok. Please look at what did the commit 1b3922a8bc74 do? It re-ran the 
> btrfs_find_device(seed_devs,..., false), which anyway the 
> btrfs_find_device(sprout_devs,..., true) has run just before. In both of 
> these btrfs_find_device() runs, the dev returned will be the same. The 
> fix makes no sense to the problem as in the commit.
> 
> 
> > 
> >> So this patch removes the check dev->disk_total_bytes == 0 in the
> >> function verify_one_dev_extent(), which it does nothing in it.
> > 
> > Removing a check that supposedly does notghing, but the referenced
> > commit says otherwise.
> > 
> 
> IMO the reason for the problem found in that commit was wrong. Qu 
> commit's email thread has some discussion. But nothing more on the problem.
> 
> Also Nikolay had the same question here was my reply.
> https://www.spinics.net/lists/linux-btrfs/msg105645.html

If the same question is asked then it's missing in the changelog, that's
where people will want to read it in the future and not in the
mailinglist archives.

> >> And take this opportunity to introduce a check if the device::total_bytes
> >> is more than the max device size in read_one_dev().
> > 
> > If this is not related to the the check removal, then it should be an
> > independent patch explaing in full what is being fixed. As I read it
> > this should be independent as it's checking the upper bound.
> > 
> 
>   That came from the Josef comments, please refer to the v1 email 
> discussion. Most of the above concerns are already discussed there. I am 
> ok to move it to a new patch if required.

And again. Same questions still unanswered, the whole point of the
iterations is to add what's been found unclear or missing. I've added it
for now but next time I'd prefer not to have to.

[PATCH RESEND v2 2/3] btrfs: fix btrfs_find_device unused arg seed

[Anand Jain]

commit 343694eee8d8 (btrfs: switch seed device to list api), missed to
check if the last arg (seed) is true in the function btrfs_find_device().
This arg tells whether to traverse through the seed device list or not.

This means after the above commit the arg is always true, and the parent
function which set this arg to false aren't effective.

So we don't worry about the parent functions which set the last arg to
true, instead there is only one parent with calling btrfs_find_device
with the last arg false in device_list_add().

But in fact, even the device_list_add() has no purpose that it has to set
the last arg to false. Because the fs_devices always points to the
device's fs_devices. So with the devid+uuid matching, it shall find the
btrfs_device and returns. So naturally, it won't traverse through the
seed fs_devices (if) present.

So this patch makes it official that we don't need the last arg in the
function btrfs_find_device() and it shall always traverse through the
seed device list.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
---
v2: -
 fs/btrfs/dev-replace.c |  4 ++--
 fs/btrfs/ioctl.c       |  4 ++--
 fs/btrfs/scrub.c       |  4 ++--
 fs/btrfs/volumes.c     | 22 ++++++++++------------
 fs/btrfs/volumes.h     |  2 +-
 5 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 5b9e3f3ace22..ffab2758f991 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -149,10 +149,10 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
 		dev_replace->srcdev = btrfs_find_device(fs_info->fs_devices,
-						src_devid, NULL, NULL, true);
+						src_devid, NULL, NULL);
 		dev_replace->tgtdev = btrfs_find_device(fs_info->fs_devices,
 							BTRFS_DEV_REPLACE_DEVID,
-							NULL, NULL, true);
+							NULL, NULL);
 		/*
 		 * allow 'btrfs dev replace_cancel' if src/tgt device is
 		 * missing
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 69a384145dc6..36e09a22068a 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1678,7 +1678,7 @@ static noinline int btrfs_ioctl_resize(struct file *file,
 		btrfs_info(fs_info, "resizing devid %llu", devid);
 	}
 
-	device = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true);
+	device = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
 	if (!device) {
 		btrfs_info(fs_info, "resizer unable to find device %llu",
 			   devid);
@@ -3321,7 +3321,7 @@ static long btrfs_ioctl_dev_info(struct btrfs_fs_info *fs_info,
 
 	rcu_read_lock();
 	dev = btrfs_find_device(fs_info->fs_devices, di_args->devid, s_uuid,
-				NULL, true);
+				NULL);
 
 	if (!dev) {
 		ret = -ENODEV;
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 58cd3278fbfe..c85b493380da 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -3858,7 +3858,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, devid, NULL, NULL, true);
+	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
 	if (!dev || (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state) &&
 		     !is_dev_replace)) {
 		mutex_unlock(&fs_info->fs_devices->device_list_mutex);
@@ -4035,7 +4035,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, devid, NULL, NULL, true);
+	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
 	if (dev)
 		sctx = dev->scrub_ctx;
 	if (sctx)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e615ca393aab..04ef251d885e 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -822,7 +822,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 	} else {
 		mutex_lock(&fs_devices->device_list_mutex);
 		device = btrfs_find_device(fs_devices, devid,
-				disk_super->dev_item.uuid, NULL, false);
+				disk_super->dev_item.uuid, NULL);
 
 		/*
 		 * If this disk has been pulled into an fs devices created by
@@ -2300,10 +2300,10 @@ static struct btrfs_device *btrfs_find_device_by_path(
 	dev_uuid = disk_super->dev_item.uuid;
 	if (btrfs_fs_incompat(fs_info, METADATA_UUID))
 		device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
-					   disk_super->metadata_uuid, true);
+					   disk_super->metadata_uuid);
 	else
 		device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
-					   disk_super->fsid, true);
+					   disk_super->fsid);
 
 	btrfs_release_disk_super(disk_super);
 	if (!device)
@@ -2323,7 +2323,7 @@ struct btrfs_device *btrfs_find_device_by_devspec(
 
 	if (devid) {
 		device = btrfs_find_device(fs_info->fs_devices, devid, NULL,
-					   NULL, true);
+					   NULL);
 		if (!device)
 			return ERR_PTR(-ENOENT);
 		return device;
@@ -2472,7 +2472,7 @@ static int btrfs_finish_sprout(struct btrfs_trans_handle *trans)
 		read_extent_buffer(leaf, fs_uuid, btrfs_device_fsid(dev_item),
 				   BTRFS_FSID_SIZE);
 		device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
-					   fs_uuid, true);
+					   fs_uuid);
 		BUG_ON(!device); /* Logic error */
 
 		if (device->fs_devices->seeding) {
@@ -6465,8 +6465,7 @@ blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
  * If @seed is true, traverse through the seed devices.
  */
 struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
-				       u64 devid, u8 *uuid, u8 *fsid,
-				       bool seed)
+				       u64 devid, u8 *uuid, u8 *fsid)
 {
 	struct btrfs_device *device;
 	struct btrfs_fs_devices *seed_devs;
@@ -6673,7 +6672,7 @@ static int read_one_chunk(struct btrfs_key *key, struct extent_buffer *leaf,
 				   btrfs_stripe_dev_uuid_nr(chunk, i),
 				   BTRFS_UUID_SIZE);
 		map->stripes[i].dev = btrfs_find_device(fs_info->fs_devices,
-							devid, uuid, NULL, true);
+							devid, uuid, NULL);
 		if (!map->stripes[i].dev &&
 		    !btrfs_test_opt(fs_info, DEGRADED)) {
 			free_extent_map(em);
@@ -6812,7 +6811,7 @@ static int read_one_dev(struct extent_buffer *leaf,
 	}
 
 	device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
-				   fs_uuid, true);
+				   fs_uuid);
 	if (!device) {
 		if (!btrfs_test_opt(fs_info, DEGRADED)) {
 			btrfs_report_missing_device(fs_info, devid,
@@ -7479,8 +7478,7 @@ int btrfs_get_dev_stats(struct btrfs_fs_info *fs_info,
 	int i;
 
 	mutex_lock(&fs_devices->device_list_mutex);
-	dev = btrfs_find_device(fs_info->fs_devices, stats->devid, NULL, NULL,
-				true);
+	dev = btrfs_find_device(fs_info->fs_devices, stats->devid, NULL, NULL);
 	mutex_unlock(&fs_devices->device_list_mutex);
 
 	if (!dev) {
@@ -7611,7 +7609,7 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
 	}
 
 	/* Make sure no dev extent is beyond device bondary */
-	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true);
+	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
 	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 34fd440fd30b..bd95cea85a65 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -466,7 +466,7 @@ 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(struct btrfs_fs_devices *fs_devices,
-				       u64 devid, u8 *uuid, u8 *fsid, bool seed);
+				       u64 devid, u8 *uuid, u8 *fsid);
 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,
-- 
2.25.1

[PATCH RESEND v2 3/3] btrfs: fix devid 0 without a replace item by failing the mount

[Anand Jain]

If there is a BTRFS_DEV_REPLACE_DEVID without a replace item, then
it means some device is trying to attack or may be corrupted. Fail the
mount so that the user can remove the attacking or fix the corrupted
device.

As of now if BTRFS_DEV_REPLACE_DEVID is present without the replace
item, in __btrfs_free_extra_devids() we determine that there is an
extra device, and free those extra devices but continue to mount the
device.
However, we were wrong in keeping tack of the rw_devices so the syzbot
testcase failed as below [1].

[1]
WARNING: CPU: 1 PID: 3612 at fs/btrfs/volumes.c:1166 close_fs_devices.part.0+0x607/0x800 fs/btrfs/volumes.c:1166
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 3612 Comm: syz-executor.2 Not tainted 5.9.0-rc4-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x198/0x1fd lib/dump_stack.c:118
 panic+0x347/0x7c0 kernel/panic.c:231
 __warn.cold+0x20/0x46 kernel/panic.c:600
 report_bug+0x1bd/0x210 lib/bug.c:198
 handle_bug+0x38/0x90 arch/x86/kernel/traps.c:234
 exc_invalid_op+0x14/0x40 arch/x86/kernel/traps.c:254
 asm_exc_invalid_op+0x12/0x20 arch/x86/include/asm/idtentry.h:536
RIP: 0010:close_fs_devices.part.0+0x607/0x800 fs/btrfs/volumes.c:1166
Code: 0f b6 04 02 84 c0 74 02 7e 33 48 8b 44 24 18 c6 80 30 01 00 00 00 48 83 c4 30 5b 5d 41 5c 41 5d 41 5e 41 5f c3 e8 99 ce 6a fe <0f> 0b e9 71 ff ff ff e8 8d ce 6a fe 0f 0b e9 20 ff ff ff e8 d1 d5
RSP: 0018:ffffc900091777e0 EFLAGS: 00010246
RAX: 0000000000040000 RBX: ffffffffffffffff RCX: ffffc9000c8b7000
RDX: 0000000000040000 RSI: ffffffff83097f47 RDI: 0000000000000007
RBP: dffffc0000000000 R08: 0000000000000001 R09: ffff8880988a187f
R10: 0000000000000000 R11: 0000000000000001 R12: ffff88809593a130
R13: ffff88809593a1ec R14: ffff8880988a1908 R15: ffff88809593a050
 close_fs_devices fs/btrfs/volumes.c:1193 [inline]
 btrfs_close_devices+0x95/0x1f0 fs/btrfs/volumes.c:1179
 open_ctree+0x4984/0x4a2d fs/btrfs/disk-io.c:3434
 btrfs_fill_super fs/btrfs/super.c:1316 [inline]
 btrfs_mount_root.cold+0x14/0x165 fs/btrfs/super.c:1672

The fix here is, when we determine that there isn't a replace item
then fail the mount if there is a replace target device (devid 0).

The reproducer and the fix looks like this.

 mkfs.btrfs -fq /dev/sda
 dd if=/dev/sda of=/dev/sdb bs=1 seek=64K skip=64K count=4096
 btrfs-sb-mod /dev/sdb dev_item.devid =0
 mount -o device=/dev/sdb /dev/sda /btrfsS
mount: /btrfs: wrong fs type, bad option, bad superblock on /dev/sda, missing codepage or helper program, or other error.

[ 3526.708108] BTRFS error (device sdb): found replace target device without a replace item
[ 3526.709152] BTRFS error (device sdb): failed to init dev_replace: -5
[ 3526.715623] BTRFS error (device sdb): open_ctree failed

Cc: josef@toxicpanda.com
Reported-by: syzbot+4cfe71a4da060be47502@syzkaller.appspotmail.com
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
(changle log reproducer updated).

Conflicts with the patches
 btrfs: drop never met condition of disk_total_bytes == 0
 btrfs: fix btrfs_find_device unused arg seed
If these patches aren't integrated yet, then please add the last arg in
the function btrfs_find_device(). Any value is fine as it doesn't care.

Dropped the idea of fstests for now as the test case requires btrfs-sb-mod.
And there aren't any fstests yet which shall carry 4k superblock in its script
not sure if it's a good idea.

Instead, we could rely on sysbot for now.

v2: changed title
    old: btrfs: fix rw_devices count in __btrfs_free_extra_devids

    In btrfs_init_dev_replace() try to match the presence of replace_item
    to the BTRFS_DEV_REPLACE_DEVID device. If fails then fail the
    mount. So drop the similar check in __btrfs_free_extra_devids().

 fs/btrfs/dev-replace.c | 26 ++++++++++++++++++++++++--
 fs/btrfs/volumes.c     | 26 +++++++-------------------
 2 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index ffab2758f991..8b3935757dc1 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -91,6 +91,17 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
 	ret = btrfs_search_slot(NULL, dev_root, &key, path, 0, 0);
 	if (ret) {
 no_valid_dev_replace_entry_found:
+		/*
+		 * 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,
+				      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
+			btrfs_err(fs_info,
+			"found replace target device without a replace item");
+			ret = -EIO;
+			goto out;
+		}
 		ret = 0;
 		dev_replace->replace_state =
 			BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED;
@@ -143,8 +154,19 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED:
-		dev_replace->srcdev = NULL;
-		dev_replace->tgtdev = NULL;
+		/*
+		 * 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,
+				      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
+			btrfs_err(fs_info,
+			"replace devid present without an active replace item");
+			ret = -EIO;
+		} else {
+			dev_replace->srcdev = NULL;
+			dev_replace->tgtdev = NULL;
+		}
 		break;
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 04ef251d885e..9dd55a6f38de 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1056,22 +1056,13 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
 			continue;
 		}
 
-		if (device->devid == BTRFS_DEV_REPLACE_DEVID) {
-			/*
-			 * In the first step, keep the device which has
-			 * the correct fsid and the devid that is used
-			 * for the dev_replace procedure.
-			 * In the second step, the dev_replace state is
-			 * read from the device tree and it is known
-			 * whether the procedure is really active or
-			 * not, which means whether this device is
-			 * used or whether it should be removed.
-			 */
-			if (step == 0 || test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
-						  &device->dev_state)) {
-				continue;
-			}
-		}
+		/*
+		 * We have already validated the presence of BTRFS_DEV_REPLACE_DEVID,
+		 * in btrfs_init_dev_replace() so just continue.
+		 */
+		if (device->devid == BTRFS_DEV_REPLACE_DEVID)
+			continue;
+
 		if (device->bdev) {
 			blkdev_put(device->bdev, device->mode);
 			device->bdev = NULL;
@@ -1080,9 +1071,6 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
 		if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
 			list_del_init(&device->dev_alloc_list);
 			clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
-			if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
-				      &device->dev_state))
-				fs_devices->rw_devices--;
 		}
 		list_del_init(&device->dev_list);
 		fs_devices->num_devices--;
-- 
2.25.1

Re: [PATCH RESEND v2 0/3] cleanup btrfs_find_device and fix sysbot warning

[David Sterba]

On Tue, Nov 03, 2020 at 01:49:41PM +0800, Anand Jain wrote:
> Patch 1 and 2 are cleanups. They were sent before with the cover-letter
> as below.
>  [PATCH RESEND 0/2] fix verify_one_dev_extent and btrfs_find_device
> 
> Patch 3 is a fix for the Warning reported by sysbot. This also was sent
> before with the cover-letter titled as below.
>   [PATCH 0/1] handle attacking device with devid=0
> 
> To avoid conflict fixes, please apply these patches in the order it is
> sent here. But the sets are not related.
> 
> Both of these patchsets are at v2 at the latest. So I am marking all of
> them as v2. The individual changes are in the patches.
> 
> Also earlier resend patches were threaded under its previous cover-letter
> (though --in-reply-to was not used to generate/send those patches). So I am
> trying to resend these patches again, hopefully, they are not threaded to
> the older series again.
> 
> Anand Jain (3):
>   btrfs: drop never met condition of disk_total_bytes == 0
>   btrfs: fix btrfs_find_device unused arg seed
>   btrfs: fix devid 0 without a replace item by failing the mount

Now added to misc-next, thanks.