[For 5.0-rc PATCH] btrfs: Use real device structure to verify dev extent

[For 5.0-rc PATCH] btrfs: Use real device structure to verify dev extent

[Qu Wenruo]

[BUG]
Linux v5.0-rc1 will fail fstests/btrfs/163 with the following kernel
message:

  BTRFS error (device dm-6): dev extent devid 1 physical offset 13631488 len 8388608 is beyond device boundary 0
  BTRFS error (device dm-6): failed to verify dev extents against chunks: -117
  BTRFS error (device dm-6): open_ctree failed

[CAUSE]
Commit cf90d884b347 ("btrfs: Introduce mount time chunk <-> dev extent
mapping check") introduced strict check on dev extents.

We use btrfs_find_device() with dev uuid and fs uuid set to NULL, and
only dependent on @devid to find the real device.

For seed devices, we call clone_fs_devices() in open_seed_devices() to
allow us search seed devices directly.

However clone_fs_devices() just populates devices with devid and dev
uuid, without populating other essential members, like disk_total_bytes.

This makes any device returned by btrfs_find_device(fs_info, devid,
NULL, NULL) is just a dummy, with 0 disk_total_bytes, and any dev
extents on the seed device will not pass the device boundary check.

[FIX]
This patch will try to verify the device returned by btrfs_find_device()
and if it's a dummy then re-search in seed devices.

Reported-by: Filipe Manana <fdmanana@suse.com>
Fixes: cf90d884b347 ("btrfs: Introduce mount time chunk <-> dev extent mapping check")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/volumes.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 2576b1a379c9..3e4f8f88353e 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -7825,6 +7825,18 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
 		ret = -EUCLEAN;
 		goto out;
 	}
+
+	/* It's possible this device is a dummy for seed device */
+	if (dev->disk_total_bytes == 0) {
+		dev = find_device(fs_info->fs_devices->seed, devid, NULL);
+		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.20.1

Re: [For 5.0-rc PATCH] btrfs: Use real device structure to verify dev extent

[Filipe Manana]

On Tue, Jan 8, 2019 at 6:11 AM Qu Wenruo <wqu@suse.com> wrote:
>
> [BUG]
> Linux v5.0-rc1 will fail fstests/btrfs/163 with the following kernel
> message:
>
>   BTRFS error (device dm-6): dev extent devid 1 physical offset 13631488 len 8388608 is beyond device boundary 0
>   BTRFS error (device dm-6): failed to verify dev extents against chunks: -117
>   BTRFS error (device dm-6): open_ctree failed
>
> [CAUSE]
> Commit cf90d884b347 ("btrfs: Introduce mount time chunk <-> dev extent
> mapping check") introduced strict check on dev extents.
>
> We use btrfs_find_device() with dev uuid and fs uuid set to NULL, and
> only dependent on @devid to find the real device.
>
> For seed devices, we call clone_fs_devices() in open_seed_devices() to
> allow us search seed devices directly.
>
> However clone_fs_devices() just populates devices with devid and dev
> uuid, without populating other essential members, like disk_total_bytes.
>
> This makes any device returned by btrfs_find_device(fs_info, devid,
> NULL, NULL) is just a dummy, with 0 disk_total_bytes, and any dev
> extents on the seed device will not pass the device boundary check.
>
> [FIX]
> This patch will try to verify the device returned by btrfs_find_device()
> and if it's a dummy then re-search in seed devices.
>
> Reported-by: Filipe Manana <fdmanana@suse.com>
> Fixes: cf90d884b347 ("btrfs: Introduce mount time chunk <-> dev extent mapping check")
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/volumes.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 2576b1a379c9..3e4f8f88353e 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -7825,6 +7825,18 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
>                 ret = -EUCLEAN;
>                 goto out;
>         }
> +
> +       /* It's possible this device is a dummy for seed device */
> +       if (dev->disk_total_bytes == 0) {
> +               dev = find_device(fs_info->fs_devices->seed, devid, NULL);
> +               if (!dev) {
> +                       btrfs_err(fs_info, "failed to find seed devid %llu",
> +                                 devid);
> +                       ret = -EUCLEAN;
> +                       goto out;
> +               }
> +       }

Why just not pass the FSID (can be taken from fs_info->super_copy) to
the previous call to btrfs_find_device()?
It's a lot simpler.

> +
>         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.20.1
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [For 5.0-rc PATCH] btrfs: Use real device structure to verify dev extent

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 3138 bytes --]



On 2019/1/8 下午7:16, Filipe Manana wrote:
> On Tue, Jan 8, 2019 at 6:11 AM Qu Wenruo <wqu@suse.com> wrote:
>>
>> [BUG]
>> Linux v5.0-rc1 will fail fstests/btrfs/163 with the following kernel
>> message:
>>
>>   BTRFS error (device dm-6): dev extent devid 1 physical offset 13631488 len 8388608 is beyond device boundary 0
>>   BTRFS error (device dm-6): failed to verify dev extents against chunks: -117
>>   BTRFS error (device dm-6): open_ctree failed
>>
>> [CAUSE]
>> Commit cf90d884b347 ("btrfs: Introduce mount time chunk <-> dev extent
>> mapping check") introduced strict check on dev extents.
>>
>> We use btrfs_find_device() with dev uuid and fs uuid set to NULL, and
>> only dependent on @devid to find the real device.
>>
>> For seed devices, we call clone_fs_devices() in open_seed_devices() to
>> allow us search seed devices directly.
>>
>> However clone_fs_devices() just populates devices with devid and dev
>> uuid, without populating other essential members, like disk_total_bytes.
>>
>> This makes any device returned by btrfs_find_device(fs_info, devid,
>> NULL, NULL) is just a dummy, with 0 disk_total_bytes, and any dev
>> extents on the seed device will not pass the device boundary check.
>>
>> [FIX]
>> This patch will try to verify the device returned by btrfs_find_device()
>> and if it's a dummy then re-search in seed devices.
>>
>> Reported-by: Filipe Manana <fdmanana@suse.com>
>> Fixes: cf90d884b347 ("btrfs: Introduce mount time chunk <-> dev extent mapping check")
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/volumes.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 2576b1a379c9..3e4f8f88353e 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -7825,6 +7825,18 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
>>                 ret = -EUCLEAN;
>>                 goto out;
>>         }
>> +
>> +       /* It's possible this device is a dummy for seed device */
>> +       if (dev->disk_total_bytes == 0) {
>> +               dev = find_device(fs_info->fs_devices->seed, devid, NULL);
>> +               if (!dev) {
>> +                       btrfs_err(fs_info, "failed to find seed devid %llu",
>> +                                 devid);
>> +                       ret = -EUCLEAN;
>> +                       goto out;
>> +               }
>> +       }
> 
> Why just not pass the FSID (can be taken from fs_info->super_copy) to
> the previous call to btrfs_find_device()?
> It's a lot simpler.

Then btrfs_find_device() will just return NULL.
We still need to verify the dev extent of the seed device not to exceed
seed device boundary.

What we need is device uuid, however we can't easily get that dev uuid
from dev extent item.

Thanks,
Qu

> 
>> +
>>         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.20.1
>>
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [For 5.0-rc PATCH] btrfs: Use real device structure to verify dev extent

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 4465 bytes --]



On 2019/1/8 下午7:45, Qu Wenruo wrote:
> 
> 
> On 2019/1/8 下午7:16, Filipe Manana wrote:
>> On Tue, Jan 8, 2019 at 6:11 AM Qu Wenruo <wqu@suse.com> wrote:
>>>
>>> [BUG]
>>> Linux v5.0-rc1 will fail fstests/btrfs/163 with the following kernel
>>> message:
>>>
>>>   BTRFS error (device dm-6): dev extent devid 1 physical offset 13631488 len 8388608 is beyond device boundary 0
>>>   BTRFS error (device dm-6): failed to verify dev extents against chunks: -117
>>>   BTRFS error (device dm-6): open_ctree failed
>>>
>>> [CAUSE]
>>> Commit cf90d884b347 ("btrfs: Introduce mount time chunk <-> dev extent
>>> mapping check") introduced strict check on dev extents.
>>>
>>> We use btrfs_find_device() with dev uuid and fs uuid set to NULL, and
>>> only dependent on @devid to find the real device.
>>>
>>> For seed devices, we call clone_fs_devices() in open_seed_devices() to
>>> allow us search seed devices directly.
>>>
>>> However clone_fs_devices() just populates devices with devid and dev
>>> uuid, without populating other essential members, like disk_total_bytes.
>>>
>>> This makes any device returned by btrfs_find_device(fs_info, devid,
>>> NULL, NULL) is just a dummy, with 0 disk_total_bytes, and any dev
>>> extents on the seed device will not pass the device boundary check.
>>>
>>> [FIX]
>>> This patch will try to verify the device returned by btrfs_find_device()
>>> and if it's a dummy then re-search in seed devices.
>>>
>>> Reported-by: Filipe Manana <fdmanana@suse.com>
>>> Fixes: cf90d884b347 ("btrfs: Introduce mount time chunk <-> dev extent mapping check")
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  fs/btrfs/volumes.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index 2576b1a379c9..3e4f8f88353e 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -7825,6 +7825,18 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
>>>                 ret = -EUCLEAN;
>>>                 goto out;
>>>         }
>>> +
>>> +       /* It's possible this device is a dummy for seed device */
>>> +       if (dev->disk_total_bytes == 0) {
>>> +               dev = find_device(fs_info->fs_devices->seed, devid, NULL);
>>> +               if (!dev) {
>>> +                       btrfs_err(fs_info, "failed to find seed devid %llu",
>>> +                                 devid);
>>> +                       ret = -EUCLEAN;
>>> +                       goto out;
>>> +               }
>>> +       }
>>
>> Why just not pass the FSID (can be taken from fs_info->super_copy) to
>> the previous call to btrfs_find_device()?
>> It's a lot simpler.
> 
> Then btrfs_find_device() will just return NULL.
> We still need to verify the dev extent of the seed device not to exceed
> seed device boundary.

OK, it's not the case. Even we can pass correct dev uuid into
btrfs_find_device(), it will still give us the dummy device, with 0
disk_total_bytes.

The problem is, we have a screwed up fs_info->fs_devices for seed device
from the very beginning.

For a seeding fs, devid 1 is seed device and devid 2 is real rw device.
Then our fs_devices looks like:

fs_info->fs_devices
|- devices
|  |- devid 1
|  |  dummy, devid 1 dev uuid, any else is unpopulated
|  |  created by open_seed_devices()-> clone_fs_devices()
|  |- devid 2
|     real one, devid 2 dev uuid, everything is good
|- seeding
   |- devices
      |- devid 1
         real seed device.

So, at btrfs_find_device() call time, we must pass the *seeding* fsid,
to locate the real seed device.
Or we can only get a dummy.

This patch is just a quick and dirty fix.

The correct way to fix is to not screw up fs_devices any more with such
meaningless dummy in fs_devices.
Especially when btrfs_find_device() has already handled such case.

I don't understand why we do such meaningless clone_fs_devices() call,
but I don't think the proper fix may catch the v5.0 window.

Thanks,
Qu


> 
> What we need is device uuid, however we can't easily get that dev uuid
> from dev extent item.
> 
> Thanks,
> Qu
> 
>>
>>> +
>>>         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.20.1
>>>
>>
>>
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [For 5.0-rc PATCH] btrfs: Use real device structure to verify dev extent

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2515 bytes --]

Gentle ping.

Although this is just a dirty fix, it would be much better to avoid seed
device users yelling at me.

Thanks,
Qu

On 2019/1/8 下午2:08, Qu Wenruo wrote:
> [BUG]
> Linux v5.0-rc1 will fail fstests/btrfs/163 with the following kernel
> message:
> 
>   BTRFS error (device dm-6): dev extent devid 1 physical offset 13631488 len 8388608 is beyond device boundary 0
>   BTRFS error (device dm-6): failed to verify dev extents against chunks: -117
>   BTRFS error (device dm-6): open_ctree failed
> 
> [CAUSE]
> Commit cf90d884b347 ("btrfs: Introduce mount time chunk <-> dev extent
> mapping check") introduced strict check on dev extents.
> 
> We use btrfs_find_device() with dev uuid and fs uuid set to NULL, and
> only dependent on @devid to find the real device.
> 
> For seed devices, we call clone_fs_devices() in open_seed_devices() to
> allow us search seed devices directly.
> 
> However clone_fs_devices() just populates devices with devid and dev
> uuid, without populating other essential members, like disk_total_bytes.
> 
> This makes any device returned by btrfs_find_device(fs_info, devid,
> NULL, NULL) is just a dummy, with 0 disk_total_bytes, and any dev
> extents on the seed device will not pass the device boundary check.
> 
> [FIX]
> This patch will try to verify the device returned by btrfs_find_device()
> and if it's a dummy then re-search in seed devices.
> 
> Reported-by: Filipe Manana <fdmanana@suse.com>
> Fixes: cf90d884b347 ("btrfs: Introduce mount time chunk <-> dev extent mapping check")
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/volumes.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 2576b1a379c9..3e4f8f88353e 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -7825,6 +7825,18 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
>  		ret = -EUCLEAN;
>  		goto out;
>  	}
> +
> +	/* It's possible this device is a dummy for seed device */
> +	if (dev->disk_total_bytes == 0) {
> +		dev = find_device(fs_info->fs_devices->seed, devid, NULL);
> +		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",
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [For 5.0-rc PATCH] btrfs: Use real device structure to verify dev extent

[David Sterba]

On Tue, Jan 08, 2019 at 02:08:18PM +0800, Qu Wenruo wrote:
> [BUG]
> Linux v5.0-rc1 will fail fstests/btrfs/163 with the following kernel
> message:
> 
>   BTRFS error (device dm-6): dev extent devid 1 physical offset 13631488 len 8388608 is beyond device boundary 0
>   BTRFS error (device dm-6): failed to verify dev extents against chunks: -117
>   BTRFS error (device dm-6): open_ctree failed
> 
> [CAUSE]
> Commit cf90d884b347 ("btrfs: Introduce mount time chunk <-> dev extent
> mapping check") introduced strict check on dev extents.
> 
> We use btrfs_find_device() with dev uuid and fs uuid set to NULL, and
> only dependent on @devid to find the real device.
> 
> For seed devices, we call clone_fs_devices() in open_seed_devices() to
> allow us search seed devices directly.
> 
> However clone_fs_devices() just populates devices with devid and dev
> uuid, without populating other essential members, like disk_total_bytes.

That's quite fragile but works as a quick fix, queued for 5.0-rc,
thanks.