| Message ID | 4fea8a706aedf7407d6af7a545126511168e15f5.1600940809.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix verify_one_dev_extent and btrfs_find_device | expand |
On 24.09.20 г. 13:11 ч., Anand Jain wrote: > 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. > > So this patch removes the check dev->disk_total_bytes == 0 in the > function verify_one_dev_extent() > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/volumes.c | 15 --------------- > 1 file changed, 15 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 7f43ed88fffc..9be40eece8ed 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -7578,21 +7578,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; > - } > - } The commit which introduced this check states that the device with a disk_total_bytes = 0 occurs from clone_fs_devices called from open_seed. It seems the check is legit and your changelog doesn't account for that if it's safe you should provide description why is that. > - > 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", >
On 2020/9/24 下午7:48, Nikolay Borisov wrote: > > > On 24.09.20 г. 13:11 ч., Anand Jain wrote: >> 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. >> >> So this patch removes the check dev->disk_total_bytes == 0 in the >> function verify_one_dev_extent() >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> fs/btrfs/volumes.c | 15 --------------- >> 1 file changed, 15 deletions(-) >> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >> index 7f43ed88fffc..9be40eece8ed 100644 >> --- a/fs/btrfs/volumes.c >> +++ b/fs/btrfs/volumes.c >> @@ -7578,21 +7578,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; >> - } >> - } > > The commit which introduced this check states that the device with a > disk_total_bytes = 0 occurs from clone_fs_devices called from open_seed. > It seems the check is legit and your changelog doesn't account for that > if it's safe you should provide description why is that. And it would be even better to mention the fragmentation problem in the man page for btrfs-convert. The fragmentation problem is a little too complex to explain in the error message nor usage. 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", >>
On 2020/9/24 下午7:58, Qu Wenruo wrote: > > > On 2020/9/24 下午7:48, Nikolay Borisov wrote: >> >> >> On 24.09.20 г. 13:11 ч., Anand Jain wrote: >>> 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. >>> >>> So this patch removes the check dev->disk_total_bytes == 0 in the >>> function verify_one_dev_extent() >>> >>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >>> --- >>> fs/btrfs/volumes.c | 15 --------------- >>> 1 file changed, 15 deletions(-) >>> >>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >>> index 7f43ed88fffc..9be40eece8ed 100644 >>> --- a/fs/btrfs/volumes.c >>> +++ b/fs/btrfs/volumes.c >>> @@ -7578,21 +7578,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; >>> - } >>> - } >> >> The commit which introduced this check states that the device with a >> disk_total_bytes = 0 occurs from clone_fs_devices called from open_seed. >> It seems the check is legit and your changelog doesn't account for that >> if it's safe you should provide description why is that. > > And it would be even better to mention the fragmentation problem in the > man page for btrfs-convert. > > The fragmentation problem is a little too complex to explain in the > error message nor usage. Please ignore this, I replied to the wrong thread... > > 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", >>>
On 24/9/20 7:48 pm, Nikolay Borisov wrote: > > > On 24.09.20 г. 13:11 ч., Anand Jain wrote: >> 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. >> >> So this patch removes the check dev->disk_total_bytes == 0 in the >> function verify_one_dev_extent() >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> fs/btrfs/volumes.c | 15 --------------- >> 1 file changed, 15 deletions(-) >> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >> index 7f43ed88fffc..9be40eece8ed 100644 >> --- a/fs/btrfs/volumes.c >> +++ b/fs/btrfs/volumes.c >> @@ -7578,21 +7578,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; >> - } >> - } > > The commit which introduced this check states that the device with a > disk_total_bytes = 0 occurs from clone_fs_devices called from open_seed. > It seems the check is legit and your changelog doesn't account for that > if it's safe you should provide description why is that. yes the commit 1b3922a8bc74 (btrfs: Use real device structure to verify dev extent) introduced it. In Qu's analysis, it is unclear why the total_disk_bytes was 0. Theoretically, all devices (including missing and seed) marked with the BTRFS_DEV_STATE_IN_FS_METADATA flag gets the total_disk_bytes updated at fill_device_from_item(). open_ctree() btrfs_read_chunk_tree() read_one_dev() open_seed_device() fill_device_from_item() Even if verify_one_dev_extent() reports total_disk_bytes == 0, then IMO its a bug to be fixed somewhere else and not in verify_one_dev_extent() as it's just a messenger. It is never expected that a total_disk_bytes shall be zero. So it is fine to drop the total_disk_bytes == 0 check. Thanks, Anand > >> - >> 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", >>
On 25.09.20 г. 7:17 ч., Anand Jain wrote: > > > On 24/9/20 7:48 pm, Nikolay Borisov wrote: >> >> >> On 24.09.20 г. 13:11 ч., Anand Jain wrote: >>> 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. >>> >>> So this patch removes the check dev->disk_total_bytes == 0 in the >>> function verify_one_dev_extent() >>> >>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >>> --- >>> fs/btrfs/volumes.c | 15 --------------- >>> 1 file changed, 15 deletions(-) >>> >>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >>> index 7f43ed88fffc..9be40eece8ed 100644 >>> --- a/fs/btrfs/volumes.c >>> +++ b/fs/btrfs/volumes.c >>> @@ -7578,21 +7578,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; >>> - } >>> - } >> >> The commit which introduced this check states that the device with a >> disk_total_bytes = 0 occurs from clone_fs_devices called from open_seed. >> It seems the check is legit and your changelog doesn't account for that >> if it's safe you should provide description why is that. > > yes the commit 1b3922a8bc74 (btrfs: Use real device structure to verify > dev extent) introduced it. In Qu's analysis, it is unclear why the > total_disk_bytes was 0. > > Theoretically, all devices (including missing and seed) marked with the > BTRFS_DEV_STATE_IN_FS_METADATA flag gets the total_disk_bytes updated at > fill_device_from_item(). > > open_ctree() > btrfs_read_chunk_tree() > read_one_dev() > open_seed_device() This function returns the cloned fs_devices whose devices are not initialized. Later, in read_one_dev the 'device' is acquired from fs_info->fs_devices, not from the returned fs_devices : device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid, fs_uuid, true); And finally fill_device_from_item(leaf, dev_item, device); is called for the device which was found from fs_info->fs_devices and not from the returned 'fs_devices' from : fs_devices = open_seed_devices(fs_info, fs_uuid); What this means is that struct btrfs_device of devices in fs_info->seed_list is never fully initialized. > fill_device_from_item() > > Even if verify_one_dev_extent() reports total_disk_bytes == 0, then IMO > its a bug to be fixed somewhere else and not in verify_one_dev_extent() > as it's just a messenger. It is never expected that a total_disk_bytes > shall be zero. I agree, however this would involve fixing clone_fs_devices to properly initialize struct btrfs_device. I'm in favor of removing special casing. Looking closer into verify_one_dev_extent I see that the device is referenced from fs_info->fs_devices : dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true); And indeed, it seems that all devices of fs_info->fs_devices are initialized as per your above explanation. So yeah, your patch is codewise correct but the changelog is wrong: disk_total_bytes is never set for seed devices (seed devices are after all housed in fs_info->seed_list which as I explained above doesn't fully initialize btrfs_devices) A better changelog would document following invariants: 1. Seed devices don't have their disktotal bytes initialized 2. In spite of (1), verify_dev_one_extent is never called for such devices so it's fine because devices anchored at fs_info->fs_devices are always properly initialized. <snip>
On 25/9/20 2:19 pm, Nikolay Borisov wrote: > > > On 25.09.20 г. 7:17 ч., Anand Jain wrote: >> >> >> On 24/9/20 7:48 pm, Nikolay Borisov wrote: >>> >>> >>> On 24.09.20 г. 13:11 ч., Anand Jain wrote: >>>> 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. >>>> >>>> So this patch removes the check dev->disk_total_bytes == 0 in the >>>> function verify_one_dev_extent() >>>> >>>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >>>> --- >>>> fs/btrfs/volumes.c | 15 --------------- >>>> 1 file changed, 15 deletions(-) >>>> >>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >>>> index 7f43ed88fffc..9be40eece8ed 100644 >>>> --- a/fs/btrfs/volumes.c >>>> +++ b/fs/btrfs/volumes.c >>>> @@ -7578,21 +7578,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; >>>> - } >>>> - } >>> >>> The commit which introduced this check states that the device with a >>> disk_total_bytes = 0 occurs from clone_fs_devices called from open_seed. >>> It seems the check is legit and your changelog doesn't account for that >>> if it's safe you should provide description why is that. >> >> yes the commit 1b3922a8bc74 (btrfs: Use real device structure to verify >> dev extent) introduced it. In Qu's analysis, it is unclear why the >> total_disk_bytes was 0. >> >> Theoretically, all devices (including missing and seed) marked with the >> BTRFS_DEV_STATE_IN_FS_METADATA flag gets the total_disk_bytes updated at >> fill_device_from_item(). >> >> open_ctree() >> btrfs_read_chunk_tree() >> read_one_dev() >> open_seed_device() > > This function returns the cloned fs_devices whose devices are not > initialized. Later, in read_one_dev the 'device' is acquired from > fs_info->fs_devices, not from the returned fs_devices : > > device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid, > fs_uuid, true); > > > And finally fill_device_from_item(leaf, dev_item, device); is called for > the device which was found from fs_info->fs_devices and not from the > returned 'fs_devices' from : > > fs_devices = open_seed_devices(fs_info, fs_uuid); > > What this means is that struct btrfs_device of devices in > fs_info->seed_list is never fully initialized. > >> fill_device_from_item() >> >> Even if verify_one_dev_extent() reports total_disk_bytes == 0, then IMO >> its a bug to be fixed somewhere else and not in verify_one_dev_extent() >> as it's just a messenger. It is never expected that a total_disk_bytes >> shall be zero. > > I agree, however this would involve fixing clone_fs_devices to properly > initialize struct btrfs_device. I'm in favor of removing special casing. > Cleanups are welcome. But function fill_device_from_item() is already properly initializing the struct btrfs_device. clone_fs_devices() is called at more than one place, so IMO it is fair. > Looking closer into verify_one_dev_extent I see that the device is > referenced from fs_info->fs_devices : > > dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true); > > And indeed, it seems that all devices of fs_info->fs_devices are > initialized as per your above explanation. So yeah, your patch is > codewise correct but the changelog is wrong: > > disk_total_bytes is never set for seed devices (seed devices are after > all housed in fs_info->seed_list which as I explained above doesn't > fully initialize btrfs_devices) With all due respect, did you miss to check what fill_device_from_item() is doing? > A better changelog would document following invariants: > > 1. Seed devices don't have their disktotal bytes initialized > This is wrong. > 2. In spite of (1), verify_dev_one_extent is never called for such > devices so it's fine because devices anchored at fs_info->fs_devices are > always properly initialized. > Even this is wrong. Generally seed's devid is 1, and btrfs_verify_dev_extents() starts verifying from the dev object id = 1. So typically, the seed will be the first device that gets verified. As btrfs_read_chunk_tree() is called before btrfs_verify_dev_extents() so the btrfs_device is properly initialized before the verify check. 2817 int __cold open_ctree 3073 ret = btrfs_read_chunk_tree(fs_info); <-- seed init :: 3106 ret = btrfs_verify_dev_extents(fs_info); Thanks, Anand > > <snip> >
On 25.09.20 г. 10:33 ч., Anand Jain wrote: > > <snip> > > Even this is wrong. Generally seed's devid is 1, and > btrfs_verify_dev_extents() starts verifying from the dev object id = 1. > So typically, the seed will be the first device that gets verified. As > btrfs_read_chunk_tree() is called before btrfs_verify_dev_extents() so > the btrfs_device is properly initialized before the verify check. > > 2817 int __cold open_ctree > > 3073 ret = btrfs_read_chunk_tree(fs_info); <-- seed init > :: > 3106 ret = btrfs_verify_dev_extents(fs_info); Fair, I missed that btrfs_find_device can also return a device from fs_info->seed_list because it searches it as well. So this indeed means all devices are initialized by fill_device_from_item so : Reviewed-by: Nikolay Borisov <nborisov@suse.com> Still I think this mechanism ought to be explicitly described in the changelog i.e mentioning that btrfs_find_device also returns seed devices. > > > Thanks, Anand > >> >> <snip> >> >
On 25/9/20 3:56 pm, Nikolay Borisov wrote: > > > On 25.09.20 г. 10:33 ч., Anand Jain wrote: >> >> > <snip> > >> >> Even this is wrong. Generally seed's devid is 1, and >> btrfs_verify_dev_extents() starts verifying from the dev object id = 1. >> So typically, the seed will be the first device that gets verified. As >> btrfs_read_chunk_tree() is called before btrfs_verify_dev_extents() so >> the btrfs_device is properly initialized before the verify check. >> >> 2817 int __cold open_ctree >> >> 3073 ret = btrfs_read_chunk_tree(fs_info); <-- seed init >> :: >> 3106 ret = btrfs_verify_dev_extents(fs_info); > > > Fair, I missed that btrfs_find_device can also return a device from > fs_info->seed_list because it searches it as well. > So this indeed means > all devices are initialized by fill_device_from_item so : Oh. The btrfs_find_device(). Yep something was missing in what you said I wasn't sure what. But now it makes sense. > > Reviewed-by: Nikolay Borisov <nborisov@suse.com> Thanks! -Anand > Still I think this mechanism ought to be explicitly described in the > changelog i.e mentioning that btrfs_find_device also returns seed devices. > > >> >> >> Thanks, Anand >> >>> >>> <snip> >>> >>
On 9/24/20 6:11 AM, Anand Jain wrote: > 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. > > So this patch removes the check dev->disk_total_bytes == 0 in the > function verify_one_dev_extent() > > Signed-off-by: Anand Jain <anand.jain@oracle.com> Ok I understand that logically this check is incorrect, however we pull this value from the device item, which could be corrupt. I'm looking around and I don't see a similar check in any of our other code, it should probably go in check_dev_item() maybe? I think that removing this check is ok, but we need to make sure we catch the invalid case where total_disk_bytes == 0 somewhere, so please add that in the appropriate place. Thanks, Josef
On 5/10/20 9:22 pm, Josef Bacik wrote: > On 9/24/20 6:11 AM, Anand Jain wrote: >> 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. >> >> So this patch removes the check dev->disk_total_bytes == 0 in the >> function verify_one_dev_extent() >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> > > Ok I understand that logically this check is incorrect, however we pull > this value from the device item, which could be corrupt. I'm looking > around and I don't see a similar check in any of our other code, it > should probably go in check_dev_item() maybe? I think that removing > this check is ok, but we need to make sure we catch the invalid case > where total_disk_bytes == 0 somewhere, so please add that in the > appropriate place. Thanks, We could check the upper limit based on the device size. But we can't check for zero, because while removing the device if there is a power loss, we could have a device with its total_bytes = 0, that's still valid. I shall make this check in v2 in read_one_dev() as we need a valid device->bdev. But I have a question though- we have csum for everything to determine offline corruption at the time of reading, so corruptions are taken care of. An authenticated mount is a holistic fix for the crafted image problems, and systems that aren't secured appropriately may be exposed to crafted image problems. So do we have to handle each one of those corruption/crafted-image cases independently, then? Thanks, Anand > > Josef
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 7f43ed88fffc..9be40eece8ed 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -7578,21 +7578,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",
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. So this patch removes the check dev->disk_total_bytes == 0 in the function verify_one_dev_extent() Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/volumes.c | 15 --------------- 1 file changed, 15 deletions(-)