Syzbot reports a warning in close_fs_devices that happens because
fs_devices->rw_devices is not 0 after calling btrfs_close_one_device
on each device.
This happens when a writeable device is removed in
__btrfs_free_extra_devids, but the rw device count is not decremented
accordingly. So when close_fs_devices is called, the removed device is
still counted and we get an off by 1 error.
Here is one call trace that was observed:
btrfs_mount_root():
btrfs_scan_one_device():
device_list_add(); <---------------- device added
btrfs_open_devices():
open_fs_devices():
btrfs_open_one_device(); <-------- rw device count ++
btrfs_fill_super():
open_ctree():
btrfs_free_extra_devids():
__btrfs_free_extra_devids(); <--- device removed
fail_tree_roots:
btrfs_close_devices():
close_fs_devices(); <------- rw device count off by 1
Fixes: cf89af146b7e ("btrfs: dev-replace: fail mount if we don't have replace item with target device")
Reported-by: [email protected]
Tested-by: [email protected]
Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
---
fs/btrfs/volumes.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 807502cd6510..916c25371658 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1078,6 +1078,7 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
list_del_init(&device->dev_alloc_list);
clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
+ fs_devices->rw_devices--;
}
list_del_init(&device->dev_list);
fs_devices->num_devices--;
--
2.25.1
On 15.07.21 г. 13:34, Desmond Cheong Zhi Xi wrote:
> Syzbot reports a warning in close_fs_devices that happens because
> fs_devices->rw_devices is not 0 after calling btrfs_close_one_device
> on each device.
>
> This happens when a writeable device is removed in
> __btrfs_free_extra_devids, but the rw device count is not decremented
> accordingly. So when close_fs_devices is called, the removed device is
> still counted and we get an off by 1 error.
>
> Here is one call trace that was observed:
> btrfs_mount_root():
> btrfs_scan_one_device():
> device_list_add(); <---------------- device added
> btrfs_open_devices():
> open_fs_devices():
> btrfs_open_one_device(); <-------- rw device count ++
> btrfs_fill_super():
> open_ctree():
> btrfs_free_extra_devids():
> __btrfs_free_extra_devids(); <--- device removed
> fail_tree_roots:
> btrfs_close_devices():
> close_fs_devices(); <------- rw device count off by 1
>
> Fixes: cf89af146b7e ("btrfs: dev-replace: fail mount if we don't have replace item with target device")
> Reported-by: [email protected]
> Tested-by: [email protected]
> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
Is there a reliable reproducer from syzbot? Can this be turned into an
xfstest?
> ---
> fs/btrfs/volumes.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 807502cd6510..916c25371658 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1078,6 +1078,7 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
> if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
> list_del_init(&device->dev_alloc_list);
> clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
> + fs_devices->rw_devices--;
> }
> list_del_init(&device->dev_list);
> fs_devices->num_devices--;
>
On 15/07/2021 18:34, Desmond Cheong Zhi Xi wrote:
> Syzbot reports a warning in close_fs_devices that happens because
> fs_devices->rw_devices is not 0 after calling btrfs_close_one_device
> on each device.
>
> This happens when a writeable device is removed in
> __btrfs_free_extra_devids, but the rw device count is not decremented
> accordingly. So when close_fs_devices is called, the removed device is
> still counted and we get an off by 1 error.
>
> Here is one call trace that was observed:
> btrfs_mount_root():
> btrfs_scan_one_device():
> device_list_add(); <---------------- device added
> btrfs_open_devices():
> open_fs_devices():
> btrfs_open_one_device(); <-------- rw device count ++
> btrfs_fill_super():
> open_ctree():
> btrfs_free_extra_devids():
> __btrfs_free_extra_devids(); <--- device removed
> fail_tree_roots:
> btrfs_close_devices():
> close_fs_devices(); <------- rw device count off by 1
>
> Fixes: cf89af146b7e ("btrfs: dev-replace: fail mount if we don't have replace item with target device")
> Reported-by: [email protected]
> Tested-by: [email protected]
> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
> ---
> fs/btrfs/volumes.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 807502cd6510..916c25371658 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1078,6 +1078,7 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
> if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
> list_del_init(&device->dev_alloc_list);
> clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
> + fs_devices->rw_devices--;
> }
> list_del_init(&device->dev_list);
> fs_devices->num_devices--;
>
Thanks for the fix.
Looks good.
Reviewed-by: Anand Jain <[email protected]>
On 15/7/21 7:55 pm, Nikolay Borisov wrote:
>
>
> On 15.07.21 г. 13:34, Desmond Cheong Zhi Xi wrote:
>> Syzbot reports a warning in close_fs_devices that happens because
>> fs_devices->rw_devices is not 0 after calling btrfs_close_one_device
>> on each device.
>>
>> This happens when a writeable device is removed in
>> __btrfs_free_extra_devids, but the rw device count is not decremented
>> accordingly. So when close_fs_devices is called, the removed device is
>> still counted and we get an off by 1 error.
>>
>> Here is one call trace that was observed:
>> btrfs_mount_root():
>> btrfs_scan_one_device():
>> device_list_add(); <---------------- device added
>> btrfs_open_devices():
>> open_fs_devices():
>> btrfs_open_one_device(); <-------- rw device count ++
>> btrfs_fill_super():
>> open_ctree():
>> btrfs_free_extra_devids():
>> __btrfs_free_extra_devids(); <--- device removed
>> fail_tree_roots:
>> btrfs_close_devices():
>> close_fs_devices(); <------- rw device count off by 1
>>
>> Fixes: cf89af146b7e ("btrfs: dev-replace: fail mount if we don't have replace item with target device")
>> Reported-by: [email protected]
>> Tested-by: [email protected]
>> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
>
> Is there a reliable reproducer from syzbot? Can this be turned into an
> xfstest?
>
Syzbot has some reliable reproducers here:
https://syzkaller.appspot.com/bug?id=113d9a01cbe0af3e291633ba7a7a3e983b86c3c0
Seems like it constructs two images in-memory then mounts them. I'm not
sure if that's amenable to be converted into an xfstest?
>> ---
>> fs/btrfs/volumes.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 807502cd6510..916c25371658 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1078,6 +1078,7 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
>> if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
>> list_del_init(&device->dev_alloc_list);
>> clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
>> + fs_devices->rw_devices--;
>> }
>> list_del_init(&device->dev_list);
>> fs_devices->num_devices--;
>>
On Thu, Jul 15, 2021 at 09:11:43PM +0800, Desmond Cheong Zhi Xi wrote:
> On 15/7/21 7:55 pm, Nikolay Borisov wrote:
> >
> >
> > On 15.07.21 г. 13:34, Desmond Cheong Zhi Xi wrote:
> >> Syzbot reports a warning in close_fs_devices that happens because
> >> fs_devices->rw_devices is not 0 after calling btrfs_close_one_device
> >> on each device.
> >>
> >> This happens when a writeable device is removed in
> >> __btrfs_free_extra_devids, but the rw device count is not decremented
> >> accordingly. So when close_fs_devices is called, the removed device is
> >> still counted and we get an off by 1 error.
> >>
> >> Here is one call trace that was observed:
> >> btrfs_mount_root():
> >> btrfs_scan_one_device():
> >> device_list_add(); <---------------- device added
> >> btrfs_open_devices():
> >> open_fs_devices():
> >> btrfs_open_one_device(); <-------- rw device count ++
> >> btrfs_fill_super():
> >> open_ctree():
> >> btrfs_free_extra_devids():
> >> __btrfs_free_extra_devids(); <--- device removed
> >> fail_tree_roots:
> >> btrfs_close_devices():
> >> close_fs_devices(); <------- rw device count off by 1
> >>
> >> Fixes: cf89af146b7e ("btrfs: dev-replace: fail mount if we don't have replace item with target device")
> >> Reported-by: [email protected]
> >> Tested-by: [email protected]
> >> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
> >
> > Is there a reliable reproducer from syzbot? Can this be turned into an
> > xfstest?
> >
>
> Syzbot has some reliable reproducers here:
> https://syzkaller.appspot.com/bug?id=113d9a01cbe0af3e291633ba7a7a3e983b86c3c0
>
> Seems like it constructs two images in-memory then mounts them. I'm not
> sure if that's amenable to be converted into an xfstest?
It would need to be an image from the time the warning is reproduced,
I'm not sure how much timing is also important. But iirc adding raw test
images to fstests was not welcome, so it would have to be a reproducer
and given that the syzkaller source is not human readable I'm not sure
it would be welcome either.
Maybe there's some middle ground when the image is created by mkfs and
filled with the data and then the mount loop is started from shell. But
that means to untangle the C reproducer.
On Thu, Jul 15, 2021 at 06:34:03PM +0800, Desmond Cheong Zhi Xi wrote:
> Syzbot reports a warning in close_fs_devices that happens because
> fs_devices->rw_devices is not 0 after calling btrfs_close_one_device
> on each device.
>
> This happens when a writeable device is removed in
> __btrfs_free_extra_devids, but the rw device count is not decremented
> accordingly. So when close_fs_devices is called, the removed device is
> still counted and we get an off by 1 error.
>
> Here is one call trace that was observed:
> btrfs_mount_root():
> btrfs_scan_one_device():
> device_list_add(); <---------------- device added
> btrfs_open_devices():
> open_fs_devices():
> btrfs_open_one_device(); <-------- rw device count ++
> btrfs_fill_super():
> open_ctree():
> btrfs_free_extra_devids():
> __btrfs_free_extra_devids(); <--- device removed
> fail_tree_roots:
> btrfs_close_devices():
> close_fs_devices(); <------- rw device count off by 1
>
> Fixes: cf89af146b7e ("btrfs: dev-replace: fail mount if we don't have replace item with target device")
What this patch did in the last hunk was the rw_devices decrement, but
conditional:
@@ -1080,9 +1071,6 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
list_del_init(&device->dev_alloc_list);
clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
- if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
- &device->dev_state))
- fs_devices->rw_devices--;
}
list_del_init(&device->dev_list);
fs_devices->num_devices--;
---
> @@ -1078,6 +1078,7 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
> if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
> list_del_init(&device->dev_alloc_list);
> clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
> + fs_devices->rw_devices--;
> }
> list_del_init(&device->dev_list);
> fs_devices->num_devices--;
So should it be reinstated in the original form? The rest of
cf89af146b7e handles unexpected device replace item during mount.
Adding the decrement is correct, but right now I'm not sure about the
corner case when teh devcie has the BTRFS_DEV_STATE_REPLACE_TGT bit set.
The state machine of the device bits and counters is not trivial so
fixing it one way or the other could lead to further syzbot reports if
we don't understand the issue.
On 22/7/21 1:59 am, David Sterba wrote:
> On Thu, Jul 15, 2021 at 06:34:03PM +0800, Desmond Cheong Zhi Xi wrote:
>> Syzbot reports a warning in close_fs_devices that happens because
>> fs_devices->rw_devices is not 0 after calling btrfs_close_one_device
>> on each device.
>>
>> This happens when a writeable device is removed in
>> __btrfs_free_extra_devids, but the rw device count is not decremented
>> accordingly. So when close_fs_devices is called, the removed device is
>> still counted and we get an off by 1 error.
>>
>> Here is one call trace that was observed:
>> btrfs_mount_root():
>> btrfs_scan_one_device():
>> device_list_add(); <---------------- device added
>> btrfs_open_devices():
>> open_fs_devices():
>> btrfs_open_one_device(); <-------- rw device count ++
>> btrfs_fill_super():
>> open_ctree():
>> btrfs_free_extra_devids():
>> __btrfs_free_extra_devids(); <--- device removed
>> fail_tree_roots:
>> btrfs_close_devices():
>> close_fs_devices(); <------- rw device count off by 1
>>
>> Fixes: cf89af146b7e ("btrfs: dev-replace: fail mount if we don't have replace item with target device")
>
> What this patch did in the last hunk was the rw_devices decrement, but
> conditional:
>
> @@ -1080,9 +1071,6 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
> if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
> list_del_init(&device->dev_alloc_list);
> clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
> - if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
> - &device->dev_state))
> - fs_devices->rw_devices--;
> }
> list_del_init(&device->dev_list);
> fs_devices->num_devices--;
> ---
>
>
>> @@ -1078,6 +1078,7 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
>> if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
>> list_del_init(&device->dev_alloc_list);
>> clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
>> + fs_devices->rw_devices--;
>> }
>> list_del_init(&device->dev_list);
>> fs_devices->num_devices--;
>
> So should it be reinstated in the original form? The rest of
> cf89af146b7e handles unexpected device replace item during mount.
>
> Adding the decrement is correct, but right now I'm not sure about the
> corner case when teh devcie has the BTRFS_DEV_STATE_REPLACE_TGT bit set.
> The state machine of the device bits and counters is not trivial so
> fixing it one way or the other could lead to further syzbot reports if
> we don't understand the issue.
>
Hi David,
Thanks for raising this issue. I took a closer look and I think we don't
have to reinstate the original form because it's a historical artifact.
The short version of the story is that going by the intention of
__btrfs_free_extra_devids, we skip removing the replace target device.
Hence, by the time we've reached the decrement in question, the device
is not the replace target device and the BTRFS_DEV_STATE_REPLACE_TGT bit
should not be set.
But we should also try to understand the original intention of the code.
The check in question was first introduced in commit 8dabb7420f01
("Btrfs: change core code of btrfs to support the device replace
operations"):
> @@ -536,7 +553,8 @@ void btrfs_close_extra_devices(struct btrfs_fs_devices *fs_devices)
> if (device->writeable) {
> list_del_init(&device->dev_alloc_list);
> device->writeable = 0;
> - fs_devices->rw_devices--;
> + if (!device->is_tgtdev_for_dev_replace)
> + fs_devices->rw_devices--;
> }
> list_del_init(&device->dev_list);
> fs_devices->num_devices--;
If we take a trip back in time to this commit we see that
btrfs_dev_replace_finishing added the target device to the alloc list
without incrementing the rw_devices count. So this check was likely
originally meant to prevent under-counting of rw_devices.
However, the situation has changed, following various fixes to
rw_devices counting. Commit 63dd86fa79db ("btrfs: fix rw_devices miss
match after seed replace") added an increment to rw_devices when
replacing a seed device with a writable one in btrfs_dev_replace_finishing:
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index eea26e1b2fda..fb0a7fa2f70c 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -562,6 +562,8 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
> if (fs_info->fs_devices->latest_bdev == src_device->bdev)
> fs_info->fs_devices->latest_bdev = tgt_device->bdev;
> list_add(&tgt_device->dev_alloc_list, &fs_info->fs_devices->alloc_list);
> + if (src_device->fs_devices->seeding)
> + fs_info->fs_devices->rw_devices++;
>
> /* replace the sysfs entry */
> btrfs_kobj_rm_device(fs_info, src_device);
This was later simplified in commit 82372bc816d7 ("Btrfs: make the logic
of source device removing more clear") that simply decremented
rw_devices in btrfs_rm_dev_replace_srcdev if the replaced device was
writable. This meant that the rw_devices count could be incremented in
btrfs_dev_replace_finishing without any checks:
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index e9cbbdb72978..6f662b34ba0e 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -569,8 +569,7 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
> if (fs_info->fs_devices->latest_bdev == src_device->bdev)
> fs_info->fs_devices->latest_bdev = tgt_device->bdev;
> list_add(&tgt_device->dev_alloc_list, &fs_info->fs_devices->alloc_list);
> - if (src_device->fs_devices->seeding)
> - fs_info->fs_devices->rw_devices++;
> + fs_info->fs_devices->rw_devices++;
>
> /* replace the sysfs entry */
> btrfs_kobj_rm_device(fs_info, src_device);
Thus, given the current state of the code base, the original check is
now incorrect, because we want to decrement rw_devices as long as the
device is being removed from the alloc list.
To further convince ourselves of this, we can take a closer look at the
relation between the device with devid BTRFS_DEV_REPLACE_DEVID and the
BTRFS_DEV_STATE_REPLACE_TGT bit for devices.
BTRFS_DEV_STATE_REPLACE_TGT is set in two places:
- btrfs_init_dev_replace_tgtdev
- btrfs_init_dev_replace
In btrfs_init_dev_replace_tgtdev, the BTRFS_DEV_STATE_REPLACE_TGT bit is
set for a device allocated with devid BTRFS_DEV_REPLACE_DEVID.
In btrfs_init_dev_replace, the BTRFS_DEV_STATE_REPLACE_TGT bit is set
for the target device found with devid BTRFS_DEV_REPLACE_DEVID.
From both cases, we see that the BTRFS_DEV_STATE_REPLACE_TGT bit is set
only for the device with devid BTRFS_DEV_REPLACE_DEVID.
It follows that if a device does not have devid BTRFS_DEV_REPLACE_DEVID,
then the BTRFS_DEV_STATE_REPLACE_TGT bit will not be set.
With commit cf89af146b7e ("btrfs: dev-replace: fail mount if we don't
have replace item with target device"), we skip removing the device in
__btrfs_free_extra_devids as long as the devid is BTRFS_DEV_REPLACE_DEVID:
> - if (device->devid == BTRFS_DEV_REPLACE_DEVID) {
> - /*
> - * In the first step, keep the device which has
> - * the correct fsid and the devid that is used
> - * for the dev_replace procedure.
> - * In the second step, the dev_replace state is
> - * read from the device tree and it is known
> - * whether the procedure is really active or
> - * not, which means whether this device is
> - * used or whether it should be removed.
> - */
> - if (step == 0 || test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
> - &device->dev_state)) {
> - continue;
> - }
> - }
> + /*
> + * We have already validated the presence of BTRFS_DEV_REPLACE_DEVID,
> + * in btrfs_init_dev_replace() so just continue.
> + */
> + if (device->devid == BTRFS_DEV_REPLACE_DEVID)
> + continue;
Given the discussion above, after we fail the check for device->devid ==
BTRFS_DEV_REPLACE_DEVID, all devices from that point are not the replace
target device, and do not have the BTRFS_DEV_STATE_REPLACE_TGT bit set.
So the original check for the BTRFS_DEV_STATE_REPLACE_TGT bit before
incrementing rw_devices is not just incorrect at this point, it's also
redundant.
Of course, I would hate to introduce a hard-to-find bug with a bad
analysis, so any thoughts on this would be appreciated.
Best wishes,
Desmond
On 22/07/2021 01:59, David Sterba wrote:
> On Thu, Jul 15, 2021 at 06:34:03PM +0800, Desmond Cheong Zhi Xi wrote:
>> Syzbot reports a warning in close_fs_devices that happens because
>> fs_devices->rw_devices is not 0 after calling btrfs_close_one_device
>> on each device.
>>
>> This happens when a writeable device is removed in
>> __btrfs_free_extra_devids, but the rw device count is not decremented
>> accordingly. So when close_fs_devices is called, the removed device is
>> still counted and we get an off by 1 error.
>>
>> Here is one call trace that was observed:
>> btrfs_mount_root():
>> btrfs_scan_one_device():
>> device_list_add(); <---------------- device added
>> btrfs_open_devices():
>> open_fs_devices():
>> btrfs_open_one_device(); <-------- rw device count ++
>> btrfs_fill_super():
>> open_ctree():
>> btrfs_free_extra_devids():
>> __btrfs_free_extra_devids(); <--- device removed
>> fail_tree_roots:
>> btrfs_close_devices():
>> close_fs_devices(); <------- rw device count off by 1
>>
>> Fixes: cf89af146b7e ("btrfs: dev-replace: fail mount if we don't have replace item with target device")
>
> What this patch did in the last hunk was the rw_devices decrement, but
> conditional:
>
> @@ -1080,9 +1071,6 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
> if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
> list_del_init(&device->dev_alloc_list);
> clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
> - if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
> - &device->dev_state))
This condition was wrong.
The 1st roll of this patch which is here [1], has the details of why. As
shown below -
[1]
https://patchwork.kernel.org/project/linux-btrfs/patch/b3a0a629df98bd044a1fd5c4964f381ff6e7aa05.1600777827.git.anand.jain@oracle.com/#23640775
----
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 isn't set because
there isn't the replace-item. So rw_devices went below zero.
----
> - fs_devices->rw_devices--;
> }
> list_del_init(&device->dev_list);
> fs_devices->num_devices--;
> ---
>
>
>> @@ -1078,6 +1078,7 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
>> if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
>> list_del_init(&device->dev_alloc_list);
>> clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
>> + fs_devices->rw_devices--;
>> }
>> list_del_init(&device->dev_list);
>> fs_devices->num_devices--;
>
> So should it be reinstated in the original form?
No. The reason is the same as above.
Only the rw_devices decrement line has to be restored.
> The rest of
> cf89af146b7e handles unexpected device replace item during mount.
> Adding the decrement is correct, but right now I'm not sure about the
> corner case when teh devcie has the BTRFS_DEV_STATE_REPLACE_TGT bit set.
BTRFS_DEV_STATE_REPLACE_TGT is set (on BTRFS_DEV_REPLACE_DEVID) for two
reasons when we call replace through ioctl or during mount upon finding
a replace-device item.
> The state machine of the device bits and counters is not trivial so
> fixing it one way or the other could lead to further syzbot reports if
> we don't understand the issue.
I agree. Also, a good idea to convert this sysbot test into an xfstests
case.
Thanks, Anand
On Sun, Jul 25, 2021 at 02:19:52PM +0800, Desmond Cheong Zhi Xi wrote:
> On 22/7/21 1:59 am, David Sterba wrote:
> > On Thu, Jul 15, 2021 at 06:34:03PM +0800, Desmond Cheong Zhi Xi wrote:
> >> Syzbot reports a warning in close_fs_devices that happens because
> >> fs_devices->rw_devices is not 0 after calling btrfs_close_one_device
> >> on each device.
> >>
> >> This happens when a writeable device is removed in
> >> __btrfs_free_extra_devids, but the rw device count is not decremented
> >> accordingly. So when close_fs_devices is called, the removed device is
> >> still counted and we get an off by 1 error.
> >>
> >> Here is one call trace that was observed:
> >> btrfs_mount_root():
> >> btrfs_scan_one_device():
> >> device_list_add(); <---------------- device added
> >> btrfs_open_devices():
> >> open_fs_devices():
> >> btrfs_open_one_device(); <-------- rw device count ++
> >> btrfs_fill_super():
> >> open_ctree():
> >> btrfs_free_extra_devids():
> >> __btrfs_free_extra_devids(); <--- device removed
> >> fail_tree_roots:
> >> btrfs_close_devices():
> >> close_fs_devices(); <------- rw device count off by 1
> >>
> >> Fixes: cf89af146b7e ("btrfs: dev-replace: fail mount if we don't have replace item with target device")
> >
> > What this patch did in the last hunk was the rw_devices decrement, but
> > conditional:
> >
> > @@ -1080,9 +1071,6 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
> > if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
> > list_del_init(&device->dev_alloc_list);
> > clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
> > - if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
> > - &device->dev_state))
> > - fs_devices->rw_devices--;
> > }
> > list_del_init(&device->dev_list);
> > fs_devices->num_devices--;
> > ---
> >
> >
> >> @@ -1078,6 +1078,7 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
> >> if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
> >> list_del_init(&device->dev_alloc_list);
> >> clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
> >> + fs_devices->rw_devices--;
> >> }
> >> list_del_init(&device->dev_list);
> >> fs_devices->num_devices--;
> >
> > So should it be reinstated in the original form? The rest of
> > cf89af146b7e handles unexpected device replace item during mount.
> >
> > Adding the decrement is correct, but right now I'm not sure about the
> > corner case when teh devcie has the BTRFS_DEV_STATE_REPLACE_TGT bit set.
> > The state machine of the device bits and counters is not trivial so
> > fixing it one way or the other could lead to further syzbot reports if
> > we don't understand the issue.
> >
>
> Hi David,
>
> Thanks for raising this issue. I took a closer look and I think we don't
> have to reinstate the original form because it's a historical artifact.
>
> The short version of the story is that going by the intention of
> __btrfs_free_extra_devids, we skip removing the replace target device.
> Hence, by the time we've reached the decrement in question, the device
> is not the replace target device and the BTRFS_DEV_STATE_REPLACE_TGT bit
> should not be set.
>
> But we should also try to understand the original intention of the code.
> The check in question was first introduced in commit 8dabb7420f01
> ("Btrfs: change core code of btrfs to support the device replace
> operations"):
> > @@ -536,7 +553,8 @@ void btrfs_close_extra_devices(struct btrfs_fs_devices *fs_devices)
> > if (device->writeable) {
> > list_del_init(&device->dev_alloc_list);
> > device->writeable = 0;
> > - fs_devices->rw_devices--;
> > + if (!device->is_tgtdev_for_dev_replace)
> > + fs_devices->rw_devices--;
> > }
> > list_del_init(&device->dev_list);
> > fs_devices->num_devices--;
>
> If we take a trip back in time to this commit we see that
> btrfs_dev_replace_finishing added the target device to the alloc list
> without incrementing the rw_devices count. So this check was likely
> originally meant to prevent under-counting of rw_devices.
>
> However, the situation has changed, following various fixes to
> rw_devices counting. Commit 63dd86fa79db ("btrfs: fix rw_devices miss
> match after seed replace") added an increment to rw_devices when
> replacing a seed device with a writable one in btrfs_dev_replace_finishing:
> > diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> > index eea26e1b2fda..fb0a7fa2f70c 100644
> > --- a/fs/btrfs/dev-replace.c
> > +++ b/fs/btrfs/dev-replace.c
> > @@ -562,6 +562,8 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
> > if (fs_info->fs_devices->latest_bdev == src_device->bdev)
> > fs_info->fs_devices->latest_bdev = tgt_device->bdev;
> > list_add(&tgt_device->dev_alloc_list, &fs_info->fs_devices->alloc_list);
> > + if (src_device->fs_devices->seeding)
> > + fs_info->fs_devices->rw_devices++;
> >
> > /* replace the sysfs entry */
> > btrfs_kobj_rm_device(fs_info, src_device);
>
> This was later simplified in commit 82372bc816d7 ("Btrfs: make the logic
> of source device removing more clear") that simply decremented
> rw_devices in btrfs_rm_dev_replace_srcdev if the replaced device was
> writable. This meant that the rw_devices count could be incremented in
> btrfs_dev_replace_finishing without any checks:
> > diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> > index e9cbbdb72978..6f662b34ba0e 100644
> > --- a/fs/btrfs/dev-replace.c
> > +++ b/fs/btrfs/dev-replace.c
> > @@ -569,8 +569,7 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
> > if (fs_info->fs_devices->latest_bdev == src_device->bdev)
> > fs_info->fs_devices->latest_bdev = tgt_device->bdev;
> > list_add(&tgt_device->dev_alloc_list, &fs_info->fs_devices->alloc_list);
> > - if (src_device->fs_devices->seeding)
> > - fs_info->fs_devices->rw_devices++;
> > + fs_info->fs_devices->rw_devices++;
> >
> > /* replace the sysfs entry */
> > btrfs_kobj_rm_device(fs_info, src_device);
>
> Thus, given the current state of the code base, the original check is
> now incorrect, because we want to decrement rw_devices as long as the
> device is being removed from the alloc list.
>
> To further convince ourselves of this, we can take a closer look at the
> relation between the device with devid BTRFS_DEV_REPLACE_DEVID and the
> BTRFS_DEV_STATE_REPLACE_TGT bit for devices.
>
> BTRFS_DEV_STATE_REPLACE_TGT is set in two places:
> - btrfs_init_dev_replace_tgtdev
> - btrfs_init_dev_replace
>
> In btrfs_init_dev_replace_tgtdev, the BTRFS_DEV_STATE_REPLACE_TGT bit is
> set for a device allocated with devid BTRFS_DEV_REPLACE_DEVID.
>
> In btrfs_init_dev_replace, the BTRFS_DEV_STATE_REPLACE_TGT bit is set
> for the target device found with devid BTRFS_DEV_REPLACE_DEVID.
>
> From both cases, we see that the BTRFS_DEV_STATE_REPLACE_TGT bit is set
> only for the device with devid BTRFS_DEV_REPLACE_DEVID.
>
> It follows that if a device does not have devid BTRFS_DEV_REPLACE_DEVID,
> then the BTRFS_DEV_STATE_REPLACE_TGT bit will not be set.
>
> With commit cf89af146b7e ("btrfs: dev-replace: fail mount if we don't
> have replace item with target device"), we skip removing the device in
> __btrfs_free_extra_devids as long as the devid is BTRFS_DEV_REPLACE_DEVID:
> > - if (device->devid == BTRFS_DEV_REPLACE_DEVID) {
> > - /*
> > - * In the first step, keep the device which has
> > - * the correct fsid and the devid that is used
> > - * for the dev_replace procedure.
> > - * In the second step, the dev_replace state is
> > - * read from the device tree and it is known
> > - * whether the procedure is really active or
> > - * not, which means whether this device is
> > - * used or whether it should be removed.
> > - */
> > - if (step == 0 || test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
> > - &device->dev_state)) {
> > - continue;
> > - }
> > - }
> > + /*
> > + * We have already validated the presence of BTRFS_DEV_REPLACE_DEVID,
> > + * in btrfs_init_dev_replace() so just continue.
> > + */
> > + if (device->devid == BTRFS_DEV_REPLACE_DEVID)
> > + continue;
>
> Given the discussion above, after we fail the check for device->devid ==
> BTRFS_DEV_REPLACE_DEVID, all devices from that point are not the replace
> target device, and do not have the BTRFS_DEV_STATE_REPLACE_TGT bit set.
>
> So the original check for the BTRFS_DEV_STATE_REPLACE_TGT bit before
> incrementing rw_devices is not just incorrect at this point, it's also
> redundant.
Could you please write some condensed version of the above and resend?
The original changelog says what happends and how, the analysis here
is the actual explanation and I'd like to have that recorded. Thanks.
On 27/7/21 1:52 am, David Sterba wrote:
> On Sun, Jul 25, 2021 at 02:19:52PM +0800, Desmond Cheong Zhi Xi wrote:
>> On 22/7/21 1:59 am, David Sterba wrote:
>>> On Thu, Jul 15, 2021 at 06:34:03PM +0800, Desmond Cheong Zhi Xi wrote:
>>>> Syzbot reports a warning in close_fs_devices that happens because
>>>> fs_devices->rw_devices is not 0 after calling btrfs_close_one_device
>>>> on each device.
>>>>
>>>> This happens when a writeable device is removed in
>>>> __btrfs_free_extra_devids, but the rw device count is not decremented
>>>> accordingly. So when close_fs_devices is called, the removed device is
>>>> still counted and we get an off by 1 error.
>>>>
>>>> Here is one call trace that was observed:
>>>> btrfs_mount_root():
>>>> btrfs_scan_one_device():
>>>> device_list_add(); <---------------- device added
>>>> btrfs_open_devices():
>>>> open_fs_devices():
>>>> btrfs_open_one_device(); <-------- rw device count ++
>>>> btrfs_fill_super():
>>>> open_ctree():
>>>> btrfs_free_extra_devids():
>>>> __btrfs_free_extra_devids(); <--- device removed
>>>> fail_tree_roots:
>>>> btrfs_close_devices():
>>>> close_fs_devices(); <------- rw device count off by 1
>>>>
>>>> Fixes: cf89af146b7e ("btrfs: dev-replace: fail mount if we don't have replace item with target device")
>>>
>>> What this patch did in the last hunk was the rw_devices decrement, but
>>> conditional:
>>>
>>> @@ -1080,9 +1071,6 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
>>> if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
>>> list_del_init(&device->dev_alloc_list);
>>> clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
>>> - if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
>>> - &device->dev_state))
>>> - fs_devices->rw_devices--;
>>> }
>>> list_del_init(&device->dev_list);
>>> fs_devices->num_devices--;
>>> ---
>>>
>>>
>>>> @@ -1078,6 +1078,7 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices,
>>>> if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
>>>> list_del_init(&device->dev_alloc_list);
>>>> clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
>>>> + fs_devices->rw_devices--;
>>>> }
>>>> list_del_init(&device->dev_list);
>>>> fs_devices->num_devices--;
>>>
>>> So should it be reinstated in the original form? The rest of
>>> cf89af146b7e handles unexpected device replace item during mount.
>>>
>>> Adding the decrement is correct, but right now I'm not sure about the
>>> corner case when teh devcie has the BTRFS_DEV_STATE_REPLACE_TGT bit set.
>>> The state machine of the device bits and counters is not trivial so
>>> fixing it one way or the other could lead to further syzbot reports if
>>> we don't understand the issue.
>>>
>>
>> Hi David,
>>
>> Thanks for raising this issue. I took a closer look and I think we don't
>> have to reinstate the original form because it's a historical artifact.
>>
>> The short version of the story is that going by the intention of
>> __btrfs_free_extra_devids, we skip removing the replace target device.
>> Hence, by the time we've reached the decrement in question, the device
>> is not the replace target device and the BTRFS_DEV_STATE_REPLACE_TGT bit
>> should not be set.
>>
>> But we should also try to understand the original intention of the code.
>> The check in question was first introduced in commit 8dabb7420f01
>> ("Btrfs: change core code of btrfs to support the device replace
>> operations"):
>>> @@ -536,7 +553,8 @@ void btrfs_close_extra_devices(struct btrfs_fs_devices *fs_devices)
>>> if (device->writeable) {
>>> list_del_init(&device->dev_alloc_list);
>>> device->writeable = 0;
>>> - fs_devices->rw_devices--;
>>> + if (!device->is_tgtdev_for_dev_replace)
>>> + fs_devices->rw_devices--;
>>> }
>>> list_del_init(&device->dev_list);
>>> fs_devices->num_devices--;
>>
>> If we take a trip back in time to this commit we see that
>> btrfs_dev_replace_finishing added the target device to the alloc list
>> without incrementing the rw_devices count. So this check was likely
>> originally meant to prevent under-counting of rw_devices.
>>
>> However, the situation has changed, following various fixes to
>> rw_devices counting. Commit 63dd86fa79db ("btrfs: fix rw_devices miss
>> match after seed replace") added an increment to rw_devices when
>> replacing a seed device with a writable one in btrfs_dev_replace_finishing:
>>> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
>>> index eea26e1b2fda..fb0a7fa2f70c 100644
>>> --- a/fs/btrfs/dev-replace.c
>>> +++ b/fs/btrfs/dev-replace.c
>>> @@ -562,6 +562,8 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>>> if (fs_info->fs_devices->latest_bdev == src_device->bdev)
>>> fs_info->fs_devices->latest_bdev = tgt_device->bdev;
>>> list_add(&tgt_device->dev_alloc_list, &fs_info->fs_devices->alloc_list);
>>> + if (src_device->fs_devices->seeding)
>>> + fs_info->fs_devices->rw_devices++;
>>>
>>> /* replace the sysfs entry */
>>> btrfs_kobj_rm_device(fs_info, src_device);
>>
>> This was later simplified in commit 82372bc816d7 ("Btrfs: make the logic
>> of source device removing more clear") that simply decremented
>> rw_devices in btrfs_rm_dev_replace_srcdev if the replaced device was
>> writable. This meant that the rw_devices count could be incremented in
>> btrfs_dev_replace_finishing without any checks:
>>> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
>>> index e9cbbdb72978..6f662b34ba0e 100644
>>> --- a/fs/btrfs/dev-replace.c
>>> +++ b/fs/btrfs/dev-replace.c
>>> @@ -569,8 +569,7 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>>> if (fs_info->fs_devices->latest_bdev == src_device->bdev)
>>> fs_info->fs_devices->latest_bdev = tgt_device->bdev;
>>> list_add(&tgt_device->dev_alloc_list, &fs_info->fs_devices->alloc_list);
>>> - if (src_device->fs_devices->seeding)
>>> - fs_info->fs_devices->rw_devices++;
>>> + fs_info->fs_devices->rw_devices++;
>>>
>>> /* replace the sysfs entry */
>>> btrfs_kobj_rm_device(fs_info, src_device);
>>
>> Thus, given the current state of the code base, the original check is
>> now incorrect, because we want to decrement rw_devices as long as the
>> device is being removed from the alloc list.
>>
>> To further convince ourselves of this, we can take a closer look at the
>> relation between the device with devid BTRFS_DEV_REPLACE_DEVID and the
>> BTRFS_DEV_STATE_REPLACE_TGT bit for devices.
>>
>> BTRFS_DEV_STATE_REPLACE_TGT is set in two places:
>> - btrfs_init_dev_replace_tgtdev
>> - btrfs_init_dev_replace
>>
>> In btrfs_init_dev_replace_tgtdev, the BTRFS_DEV_STATE_REPLACE_TGT bit is
>> set for a device allocated with devid BTRFS_DEV_REPLACE_DEVID.
>>
>> In btrfs_init_dev_replace, the BTRFS_DEV_STATE_REPLACE_TGT bit is set
>> for the target device found with devid BTRFS_DEV_REPLACE_DEVID.
>>
>> From both cases, we see that the BTRFS_DEV_STATE_REPLACE_TGT bit is set
>> only for the device with devid BTRFS_DEV_REPLACE_DEVID.
>>
>> It follows that if a device does not have devid BTRFS_DEV_REPLACE_DEVID,
>> then the BTRFS_DEV_STATE_REPLACE_TGT bit will not be set.
>>
>> With commit cf89af146b7e ("btrfs: dev-replace: fail mount if we don't
>> have replace item with target device"), we skip removing the device in
>> __btrfs_free_extra_devids as long as the devid is BTRFS_DEV_REPLACE_DEVID:
>>> - if (device->devid == BTRFS_DEV_REPLACE_DEVID) {
>>> - /*
>>> - * In the first step, keep the device which has
>>> - * the correct fsid and the devid that is used
>>> - * for the dev_replace procedure.
>>> - * In the second step, the dev_replace state is
>>> - * read from the device tree and it is known
>>> - * whether the procedure is really active or
>>> - * not, which means whether this device is
>>> - * used or whether it should be removed.
>>> - */
>>> - if (step == 0 || test_bit(BTRFS_DEV_STATE_REPLACE_TGT,
>>> - &device->dev_state)) {
>>> - continue;
>>> - }
>>> - }
>>> + /*
>>> + * We have already validated the presence of BTRFS_DEV_REPLACE_DEVID,
>>> + * in btrfs_init_dev_replace() so just continue.
>>> + */
>>> + if (device->devid == BTRFS_DEV_REPLACE_DEVID)
>>> + continue;
>>
>> Given the discussion above, after we fail the check for device->devid ==
>> BTRFS_DEV_REPLACE_DEVID, all devices from that point are not the replace
>> target device, and do not have the BTRFS_DEV_STATE_REPLACE_TGT bit set.
>>
>> So the original check for the BTRFS_DEV_STATE_REPLACE_TGT bit before
>> incrementing rw_devices is not just incorrect at this point, it's also
>> redundant.
>
> Could you please write some condensed version of the above and resend?
> The original changelog says what happends and how, the analysis here
> is the actual explanation and I'd like to have that recorded. Thanks.
>
Sure thing, I'll prepare a v2 with an updated commit message. Thanks for
the feedback, David.