Re: [PATCH v2 2/4] btrfs: fix race between free_stale_devices and close_fs_devices
From: Anand Jain <hidden>
Date: 2018-08-08 12:06:53
On 08/07/2018 10:59 PM, David Sterba wrote:
On Thu, Aug 02, 2018 at 05:29:12PM +0800, Anand Jain wrote:quoted
On 08/01/2018 10:29 PM, David Sterba wrote:quoted
On Thu, Jul 26, 2018 at 02:53:32PM +0800, Anand Jain wrote:quoted
From: Anand Jain <redacted> %fs_devices can be free-ed by btrfs_free_stale_devices() when the close_fs_devices() drops fs_devices::opened to zero, but close_fs_devices tries to access the %fs_devices again without the device_list_mutex. Fix this by bringing the %fs_devices access with in the device_list_mutex.AFAICS this cannot happen anymore because the two calls are serialized by the uuid_mutex. But this was not the case when syzbot reported the problem where your patch would apply. The parallell access to opened and device list cannot happen when: * btrfs_scan_one_device that wants to call btrfs_free_stale_devices * btrfs_close_devices calls close_fs_devices Fixed by the series: btrfs: lift uuid_mutex to callers of btrfs_scan_one_device btrfs: lift uuid_mutex to callers of btrfs_open_devices btrfs: lift uuid_mutex to callers of btrfs_parse_early_options btrfs: reorder initialization before the mount locks uuid_mutex btrfs: fix mount and ioctl device scan ioctl race If there's a race I don't see, please describe in more detail.Right. There is no race with the uuid_mutex patches as above. And I just found this- can we make close be consistent with its open part. btrfs_open_devices() hold device_list_mutex before the update to fs_devices::opened. So close_fs_device() could do the same, and be theoretically correct.Or it can be the other way around, to push the device_list_mutex only around the list_sort and open_fs_devices like:
quoted hunk ↗ jump to hunk
--- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c@@ -1144,15 +1144,15 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices, lockdep_assert_held(&uuid_mutex); - mutex_lock(&fs_devices->device_list_mutex); if (fs_devices->opened) { fs_devices->opened++; ret = 0; } else { + mutex_lock(&fs_devices->device_list_mutex); list_sort(NULL, &fs_devices->devices, devid_cmp); ret = open_fs_devices(fs_devices, flags, holder); + mutex_unlock(&fs_devices->device_list_mutex); } - mutex_unlock(&fs_devices->device_list_mutex); return ret; }
Right. That it will be also true in terms of consistency. And I looked around I don't find any reason why not. Next question - btrfs_free_stale_devices() need a better way to check if the device is opened by the FS. Currently it relays on the fs_devices::opened. And while doing that, both uuid_mutex and device_list_mutex are held and since we are depend on uuid_mutex we don't need device_list_mutex. uuid_mutex is too gross could stall operation on other FSID. We can drop this patch. Do you want a patch to fix the btrfs_open_devices() consistency? Thanks, Anand