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

[Anand Jain <anand.jain@oracle.com>]

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