Re: [PATCH 1/2] btrfs: fix BUG_ON with proper error handle in find_next_devid

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

On 27/8/19 4:12 PM, Qu Wenruo wrote:
> 
> 
> On 2019/8/27 下午3:40, Anand Jain wrote:
>> In a corrupted tree if search for next devid finds the device with
>> devid = -1, then report the error -EUCLEAN back to the parent
>> function to fail gracefully.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/volumes.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 4db4a100c05b..36aa5f79fb6c 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1849,7 +1849,12 @@ static noinline int find_next_devid(struct btrfs_fs_info *fs_info,
>>   	if (ret < 0)
>>   		goto error;
>>   
>> -	BUG_ON(ret == 0); /* Corruption */
>> +	if (ret == 0) {
>> +		/* Corruption */
>> +		btrfs_err(fs_info, "corrupted chunk tree devid -1 matched");
> 
> It will never hit this branch.
> 
> As in tree checker, we have checked if the devid is so large that a
> chunk item or system chunk array can't contain one.

  That check is buggy. It assumes devid represents the num_devices,
  it does not account for the possible devid hole as created in the
  below script.

$ cat t

umount /btrfs
dev1=/dev/sdb
dev2=/dev/sdc
mkfs.btrfs -fq -dsingle -msingle $dev1
mount $dev1 /btrfs

_fail()
{
	echo $1
	exit 1
}

while true; do
	btrfs dev add -f $dev2 /btrfs || _fail "add failed"
	btrfs dev del $dev1 /btrfs || _fail "del failed"
	dev_tmp=$dev1
	dev1=$dev2
	dev2=$dev_tmp
done

-----------------------
[  185.446441] BTRFS critical (device sdb): corrupt leaf: root=3 
block=313739198464 slot=1 devid=1 invalid devid: has=507 expect=[0, 506]
[  185.446446] BTRFS error (device sdb): block=313739198464 write time 
tree block corruption detected
[  185.446556] BTRFS: error (device sdb) in 
btrfs_commit_transaction:2268: errno=-5 IO failure (Error while writing 
out transaction)
[  185.446559] BTRFS warning (device sdb): Skipping commit of aborted 
transaction.
[  185.446561] BTRFS: error (device sdb) in cleanup_transaction:1827: 
errno=-5 IO failure
-----------------------


Thanks, Anand


> That limit is way smaller than (u64)-1.
> Thus if we really have a key (DEV_ITEMS DEV_ITEM -1), it will be
> rejected by tree-checker in the first place, thus you will get a ret ==
> -EUCLEAN from previous btrfs_search_slot() call.
> 
> Thanks,
> Qu
>> +		ret = -EUCLEAN;
>> +		goto error;
>> +	}
>>   
>>   	ret = btrfs_previous_item(fs_info->chunk_root, path,
>>   				  BTRFS_DEV_ITEMS_OBJECTID,
>>
>