Re: [PATCH add reported by] btrfs: fix rw_devices count in __btrfs_free_extra_devids

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

On 22/9/20 9:08 pm, Josef Bacik wrote:
> On 9/22/20 8:33 AM, Anand Jain wrote:
>> syzbot reported a warning [1] in close_fs_devcies() which it reproduces
>> using a crafted image.
>>
>>          WARN_ON(fs_devices->rw_devices);
>>
>> The crafted image successfully creates a replace-device with the devid 0.
>> But as there isn't any replace-item. We clean the extra the devid 0, at
>> __btrfs_free_extra_devids().
>>
>> rw_devices is incremented in btrfs_open_one_device() for all write-able
>> devices except for devid == BTRFS_DEV_REPLACE_DEVID.
>> But while we clean up the extra devices in __btrfs_free_extra_devids()
>> we used the BTRFS_DEV_STATE_REPLACE_TGT flag which isn't set because
>> there isn't the replace-item. So rw_devices went below zero.
>>
>> So let __btrfs_free_extra_devids() also depend on the
>> devid != BTRFS_DEV_REPLACE_DEVID to manage the rw_devices.
>>
> 
> This is an invalid state for the fs to be in,

OK, to be more specific. There is an alien device that is pretending to 
be the replace-target (devid = 0).

 > I'd rather fix it by
 > detecting we have a devid == BTRFS_DEV_REPLACE_DEVID with no
 > corresponding dev_replace item and fail out before we get to this
 > point.  Thanks,

Yes. __btrfs_free_extra_devids() is already doing in a way the same.

------------------------------------
1040 static void __btrfs_free_extra_devids(struct btrfs_fs_devices 
*fs_devices,
::
1059 if (device->devid == BTRFS_DEV_REPLACE_DEVID) {
::
1070 if (step == 0 || test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
1071 &device->dev_state)) {
1072 continue;
1073 }
------------------------------------

OR I did not understand what do you mean.

Thanks, Anand

> 
> Josef