[PATCH 0/7] Fix locking when scanning devices

[PATCH 0/7] Fix locking when scanning devices

[David Sterba]

This patchset fixes the bugs recently reported by syzbot. I've tried to
use patches from Anand [1] to fix that but in the end there were fixes
not suitable for merging to 4.18 and my final fix took a different
approach.

In short, fs_devices::opened is protected by uuid_mutex and this mutex
can be used to exclude mount and scanning to interfere.

The fstests pass and 2 syzbot reproducers reported no problems. I'd like
to push the patchset to 4.18 but not rc2 as it's too close. I'll add the
patchset to for-next soon if there are no major problems found, but
otherwise I'm open to comments.

[1]
https://patchwork.kernel.org/patch/10446779/
https://patchwork.kernel.org/patch/10437707/ 1-6

David Sterba (7):
  btrfs: restore uuid_mutex in btrfs_open_devices
  btrfs: extend critical section when scanning a new device
  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

 fs/btrfs/super.c   | 38 +++++++++++++++++++++++++-------------
 fs/btrfs/volumes.c | 11 ++++++-----
 2 files changed, 31 insertions(+), 18 deletions(-)

-- 
2.17.0

[PATCH 1/7] btrfs: restore uuid_mutex in btrfs_open_devices

[David Sterba]

Commit 542c5908abfe84f7b4c1 ("btrfs: replace uuid_mutex by
device_list_mutex in btrfs_open_devices") switched to device_list_mutex
as we need that for the device list traversal, but we also need
uuid_mutex to protect access to fs_devices::opened to be consistent with
other users of that item.

CC: Anand Jain <anand.jain@oracle.com>
Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/volumes.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e034ad9e23b4..1da162928d1a 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1146,6 +1146,7 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 {
 	int ret;
 
+	mutex_lock(&uuid_mutex);
 	mutex_lock(&fs_devices->device_list_mutex);
 	if (fs_devices->opened) {
 		fs_devices->opened++;
@@ -1155,6 +1156,7 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 		ret = open_fs_devices(fs_devices, flags, holder);
 	}
 	mutex_unlock(&fs_devices->device_list_mutex);
+	mutex_unlock(&uuid_mutex);
 
 	return ret;
 }
-- 
2.17.0

[PATCH 2/7] btrfs: extend critical section when scanning a new device

[David Sterba]

The stale device list removal needs to be protected by device_list_mutex
too as this could delete from the list and could race with another list
modification and cause crash.

The device needs to be fully initialized before it's added to the list
so the fs_devices also need to be set under the mutex.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/volumes.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 1da162928d1a..02246f9af0a3 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -791,12 +791,11 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 		rcu_assign_pointer(device->name, name);
 
 		mutex_lock(&fs_devices->device_list_mutex);
+		device->fs_devices = fs_devices;
 		list_add_rcu(&device->dev_list, &fs_devices->devices);
 		fs_devices->num_devices++;
-		mutex_unlock(&fs_devices->device_list_mutex);
-
-		device->fs_devices = fs_devices;
 		btrfs_free_stale_devices(path, device);
+		mutex_unlock(&fs_devices->device_list_mutex);
 
 		if (disk_super->label[0])
 			pr_info("BTRFS: device label %s devid %llu transid %llu %s\n",
-- 
2.17.0

[PATCH 3/7] btrfs: lift uuid_mutex to callers of btrfs_scan_one_device

[David Sterba]

Prepartory work to fix race between mount and device scan.

The callers will have to manage the critical section, eg. mount wants to
scan and then call btrfs_open_devices without the ioctl scan walking in
and modifying the fs devices in the meantime.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/super.c   | 12 +++++++++++-
 fs/btrfs/volumes.c |  4 ++--
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 81107ad49f3a..735402ed3154 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -917,8 +917,10 @@ static int btrfs_parse_early_options(const char *options, fmode_t flags,
 				error = -ENOMEM;
 				goto out;
 			}
+			mutex_lock(&uuid_mutex);
 			error = btrfs_scan_one_device(device_name,
 					flags, holder, fs_devices);
+			mutex_unlock(&uuid_mutex);
 			kfree(device_name);
 			if (error)
 				goto out;
@@ -1539,7 +1541,9 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
 			return ERR_PTR(error);
 	}
 
+	mutex_lock(&uuid_mutex);
 	error = btrfs_scan_one_device(device_name, mode, fs_type, &fs_devices);
+	mutex_unlock(&uuid_mutex);
 	if (error)
 		goto error_sec_opts;
 
@@ -2234,15 +2238,21 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
 
 	switch (cmd) {
 	case BTRFS_IOC_SCAN_DEV:
+		mutex_lock(&uuid_mutex);
 		ret = btrfs_scan_one_device(vol->name, FMODE_READ,
 					    &btrfs_root_fs_type, &fs_devices);
+		mutex_unlock(&uuid_mutex);
 		break;
 	case BTRFS_IOC_DEVICES_READY:
+		mutex_lock(&uuid_mutex);
 		ret = btrfs_scan_one_device(vol->name, FMODE_READ,
 					    &btrfs_root_fs_type, &fs_devices);
-		if (ret)
+		if (ret) {
+			mutex_unlock(&uuid_mutex);
 			break;
+		}
 		ret = !(fs_devices->num_devices == fs_devices->total_devices);
+		mutex_unlock(&uuid_mutex);
 		break;
 	case BTRFS_IOC_GET_SUPPORTED_FEATURES:
 		ret = btrfs_ioctl_get_supported_features((void __user*)arg);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 02246f9af0a3..958bfe1a725c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1226,6 +1226,8 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 	int ret = 0;
 	u64 bytenr;
 
+	lockdep_assert_held(&uuid_mutex);
+
 	/*
 	 * we would like to check all the supers, but that would make
 	 * a btrfs mount succeed after a mkfs from a different FS.
@@ -1244,13 +1246,11 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 		goto error_bdev_put;
 	}
 
-	mutex_lock(&uuid_mutex);
 	device = device_list_add(path, disk_super);
 	if (IS_ERR(device))
 		ret = PTR_ERR(device);
 	else
 		*fs_devices_ret = device->fs_devices;
-	mutex_unlock(&uuid_mutex);
 
 	btrfs_release_disk_super(page);
 
-- 
2.17.0

[PATCH 4/7] btrfs: lift uuid_mutex to callers of btrfs_open_devices

[David Sterba]

Prepartory work to fix race between mount and device scan.

The callers will have to manage the critical section, eg.  mount wants
to scan and then call btrfs_open_devices without the ioctl scan walking
in and modifying the fs devices in the meantime.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/super.c   | 2 ++
 fs/btrfs/volumes.c | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 735402ed3154..ee82d02f5453 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1569,7 +1569,9 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
 		goto error_fs_info;
 	}
 
+	mutex_lock(&uuid_mutex);
 	error = btrfs_open_devices(fs_devices, mode, fs_type);
+	mutex_unlock(&uuid_mutex);
 	if (error)
 		goto error_fs_info;
 
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 958bfe1a725c..5336d9832ba4 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1145,7 +1145,8 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 {
 	int ret;
 
-	mutex_lock(&uuid_mutex);
+	lockdep_assert_held(&uuid_mutex);
+
 	mutex_lock(&fs_devices->device_list_mutex);
 	if (fs_devices->opened) {
 		fs_devices->opened++;
@@ -1155,7 +1156,6 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 		ret = open_fs_devices(fs_devices, flags, holder);
 	}
 	mutex_unlock(&fs_devices->device_list_mutex);
-	mutex_unlock(&uuid_mutex);
 
 	return ret;
 }
-- 
2.17.0

[PATCH 5/7] btrfs: lift uuid_mutex to callers of btrfs_parse_early_options

[David Sterba]

Prepartory work to fix race between mount and device scan.

btrfs_parse_early_options calls the device scan from mount and we'll
need to let mount completely manage the critical section.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/super.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index ee82d02f5453..e3324ddf2777 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -892,6 +892,8 @@ static int btrfs_parse_early_options(const char *options, fmode_t flags,
 	char *device_name, *opts, *orig, *p;
 	int error = 0;
 
+	lockdep_assert_held(&uuid_mutex);
+
 	if (!options)
 		return 0;
 
@@ -917,10 +919,8 @@ static int btrfs_parse_early_options(const char *options, fmode_t flags,
 				error = -ENOMEM;
 				goto out;
 			}
-			mutex_lock(&uuid_mutex);
 			error = btrfs_scan_one_device(device_name,
 					flags, holder, fs_devices);
-			mutex_unlock(&uuid_mutex);
 			kfree(device_name);
 			if (error)
 				goto out;
@@ -1528,8 +1528,10 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
 	if (!(flags & SB_RDONLY))
 		mode |= FMODE_WRITE;
 
+	mutex_lock(&uuid_mutex);
 	error = btrfs_parse_early_options(data, mode, fs_type,
 					  &fs_devices);
+	mutex_unlock(&uuid_mutex);
 	if (error) {
 		return ERR_PTR(error);
 	}
-- 
2.17.0

[PATCH 6/7] btrfs: reorder initialization before the mount locks uuid_mutex

[David Sterba]

In preparation to take a big lock, move resource initialization before
the critical section. It's not obvious from the diff, the desired order
is:

- initialize mount security options
- allocate temporary fs_info
- allocate superblock buffers

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/super.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index e3324ddf2777..1780eb41f203 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1528,14 +1528,6 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
 	if (!(flags & SB_RDONLY))
 		mode |= FMODE_WRITE;
 
-	mutex_lock(&uuid_mutex);
-	error = btrfs_parse_early_options(data, mode, fs_type,
-					  &fs_devices);
-	mutex_unlock(&uuid_mutex);
-	if (error) {
-		return ERR_PTR(error);
-	}
-
 	security_init_mnt_opts(&new_sec_opts);
 	if (data) {
 		error = parse_security_options(data, &new_sec_opts);
@@ -1543,12 +1535,6 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
 			return ERR_PTR(error);
 	}
 
-	mutex_lock(&uuid_mutex);
-	error = btrfs_scan_one_device(device_name, mode, fs_type, &fs_devices);
-	mutex_unlock(&uuid_mutex);
-	if (error)
-		goto error_sec_opts;
-
 	/*
 	 * Setup a dummy root and fs_info for test/set super.  This is because
 	 * we don't actually fill this stuff out until open_ctree, but we need
@@ -1561,8 +1547,6 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
 		goto error_sec_opts;
 	}
 
-	fs_info->fs_devices = fs_devices;
-
 	fs_info->super_copy = kzalloc(BTRFS_SUPER_INFO_SIZE, GFP_KERNEL);
 	fs_info->super_for_commit = kzalloc(BTRFS_SUPER_INFO_SIZE, GFP_KERNEL);
 	security_init_mnt_opts(&fs_info->security_opts);
@@ -1571,6 +1555,20 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
 		goto error_fs_info;
 	}
 
+	mutex_lock(&uuid_mutex);
+	error = btrfs_parse_early_options(data, mode, fs_type, &fs_devices);
+	mutex_unlock(&uuid_mutex);
+	if (error)
+		goto error_fs_info;
+
+	mutex_lock(&uuid_mutex);
+	error = btrfs_scan_one_device(device_name, mode, fs_type, &fs_devices);
+	mutex_unlock(&uuid_mutex);
+	if (error)
+		goto error_fs_info;
+
+	fs_info->fs_devices = fs_devices;
+
 	mutex_lock(&uuid_mutex);
 	error = btrfs_open_devices(fs_devices, mode, fs_type);
 	mutex_unlock(&uuid_mutex);
-- 
2.17.0

[PATCH 7/7] btrfs: fix mount and ioctl device scan ioctl race

[David Sterba]

Technically this extends the critical section covered by uuid_mutex to:

- parse early mount options -- here we can call device scan on paths
  that can be passed as 'device=/dev/...'

- scan the device passed to mount

- open the devices related to the fs_devices -- this increases
  fs_devices::opened

The race can happen when mount calls one of the scans and there's
another one called eg. by mkfs or 'btrfs dev scan':

Mount                                  Scan
-----                                  ----
scan_one_device (dev1, fsid1)
                                       scan_one_device (dev2, fsid2)
				           add the device
					   free stale devices
					       fsid1 fs_devices::opened == 0
					           find fsid1:dev1
					           free fsid1:dev1
					           if it's the last one,
					            free fs_devices of fsid1
						    too

open_devices (dev1, fsid1)
   dev1 not found

When fixed, the uuid mutex will make sure that mount will increase
fs_devices::opened and this will not be touched by the racing scan
ioctl.

Reported-and-tested-by: syzbot+909a5177749d7990ffa4@syzkaller.appspotmail.com
Reported-and-tested-by: syzbot+ceb2606025ec1cc3479c@syzkaller.appspotmail.com
Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/super.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 1780eb41f203..b13b871bc584 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1557,19 +1557,19 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
 
 	mutex_lock(&uuid_mutex);
 	error = btrfs_parse_early_options(data, mode, fs_type, &fs_devices);
-	mutex_unlock(&uuid_mutex);
-	if (error)
+	if (error) {
+		mutex_unlock(&uuid_mutex);
 		goto error_fs_info;
+	}
 
-	mutex_lock(&uuid_mutex);
 	error = btrfs_scan_one_device(device_name, mode, fs_type, &fs_devices);
-	mutex_unlock(&uuid_mutex);
-	if (error)
+	if (error) {
+		mutex_unlock(&uuid_mutex);
 		goto error_fs_info;
+	}
 
 	fs_info->fs_devices = fs_devices;
 
-	mutex_lock(&uuid_mutex);
 	error = btrfs_open_devices(fs_devices, mode, fs_type);
 	mutex_unlock(&uuid_mutex);
 	if (error)
-- 
2.17.0

Re: [PATCH 0/7] Fix locking when scanning devices

[Nikolay Borisov]

On 20.06.2018 20:51, David Sterba wrote:
> This patchset fixes the bugs recently reported by syzbot. I've tried to
> use patches from Anand [1] to fix that but in the end there were fixes
> not suitable for merging to 4.18 and my final fix took a different
> approach.
> 
> In short, fs_devices::opened is protected by uuid_mutex and this mutex
> can be used to exclude mount and scanning to interfere.
> 
> The fstests pass and 2 syzbot reproducers reported no problems. I'd like
> to push the patchset to 4.18 but not rc2 as it's too close. I'll add the
> patchset to for-next soon if there are no major problems found, but
> otherwise I'm open to comments.
> 
> [1]
> https://patchwork.kernel.org/patch/10446779/
> https://patchwork.kernel.org/patch/10437707/ 1-6
> 
> David Sterba (7):
>   btrfs: restore uuid_mutex in btrfs_open_devices
>   btrfs: extend critical section when scanning a new device
>   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
> 
>  fs/btrfs/super.c   | 38 +++++++++++++++++++++++++-------------
>  fs/btrfs/volumes.c | 11 ++++++-----
>  2 files changed, 31 insertions(+), 18 deletions(-)
> 
It's good you've added lockdep asserts in functions whose locking is
lifted to the caller. However, I'm a bit hesitant about extending the
locking section so much. I.e i parse_early_opt you are covering quite
some string manipulation functionality and only the single call to
btrfs_scan_one_device. I wonder if the whole device scanning/mounting
code could be refactored to not rely on 2 locks (device list mutex +
uuid mutex, and that's after you merged your series refactoring locking).

Anyways, the series LGTM:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Re: [PATCH 0/7] Fix locking when scanning devices

[David Sterba]

On Thu, Jun 21, 2018 at 10:48:27AM +0300, Nikolay Borisov wrote:
> 
> 
> On 20.06.2018 20:51, David Sterba wrote:
> > This patchset fixes the bugs recently reported by syzbot. I've tried to
> > use patches from Anand [1] to fix that but in the end there were fixes
> > not suitable for merging to 4.18 and my final fix took a different
> > approach.
> > 
> > In short, fs_devices::opened is protected by uuid_mutex and this mutex
> > can be used to exclude mount and scanning to interfere.
> > 
> > The fstests pass and 2 syzbot reproducers reported no problems. I'd like
> > to push the patchset to 4.18 but not rc2 as it's too close. I'll add the
> > patchset to for-next soon if there are no major problems found, but
> > otherwise I'm open to comments.
> > 
> > [1]
> > https://patchwork.kernel.org/patch/10446779/
> > https://patchwork.kernel.org/patch/10437707/ 1-6
> > 
> > David Sterba (7):
> >   btrfs: restore uuid_mutex in btrfs_open_devices
> >   btrfs: extend critical section when scanning a new device
> >   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
> > 
> >  fs/btrfs/super.c   | 38 +++++++++++++++++++++++++-------------
> >  fs/btrfs/volumes.c | 11 ++++++-----
> >  2 files changed, 31 insertions(+), 18 deletions(-)
> > 
> It's good you've added lockdep asserts in functions whose locking is
> lifted to the caller. However, I'm a bit hesitant about extending the
> locking section so much. I.e i parse_early_opt you are covering quite
> some string manipulation functionality and only the single call to
> btrfs_scan_one_device.

Are string manipulations under a lock a problem? I'd be more worried
about memory allocations or IO that could hold the lock and block other
mount or scan.

As both are very infrequent operations and callers need to wait anyway,
I don't think this will be noticeable in practice.

> I wonder if the whole device scanning/mounting
> code could be refactored to not rely on 2 locks (device list mutex +
> uuid mutex, and that's after you merged your series refactoring locking).

>From what I've seen, there's not much potential for significant changes.
We need the locks to prevent either the data structures or for the
operation exclusion. The rest would be reducing the size of the critical
sections by moving unrelated calls out.

Re: [PATCH 2/7] btrfs: extend critical section when scanning a new device

[Anand Jain]

On 06/21/2018 01:51 AM, David Sterba wrote:
> The stale device list removal needs to be protected by device_list_mutex
> too as this could delete from the list and could race with another list
> modification and cause crash.

  Its is very less likely or almost practically impossible (unless
  instrumented) to have same device with same fsid but with different
  devid, which then it would delete the device which is under the same
  fs_devices which device_list_add just operated. Otherwise in most of
  the common cases it shall delete the device which does not belong to
  the fs_devices which the device_list_add() just added a device. So
  fs_devices::device_list_mutex lock won't help.

  I have few patches to fix this. I shall send it. But then it was all
  based on the atomic volume exclusive flag which in stages I had
  plans to replace fs_info::BTRFS_FS_EXCL_OP with volume exclusive flag
  as well.

  Also we need locks to manage with in btrfs_free_stale_devices() so
  that we can reuse this code to support user land forget device feature,
  (38cf665d338fca33af4b16f9ec7cad6637fc0fec btrfs: make
  btrfs_free_stale_device() to iterate all stales).

Thanks, Anand


> The device needs to be fully initialized before it's added to the list
> so the fs_devices also need to be set under the mutex.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/volumes.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 1da162928d1a..02246f9af0a3 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -791,12 +791,11 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>   		rcu_assign_pointer(device->name, name);
>   
>   		mutex_lock(&fs_devices->device_list_mutex);
> +		device->fs_devices = fs_devices;
>   		list_add_rcu(&device->dev_list, &fs_devices->devices);
>   		fs_devices->num_devices++;
> -		mutex_unlock(&fs_devices->device_list_mutex);
> -
> -		device->fs_devices = fs_devices;
>   		btrfs_free_stale_devices(path, device);
> +		mutex_unlock(&fs_devices->device_list_mutex);
>   
>   		if (disk_super->label[0])
>   			pr_info("BTRFS: device label %s devid %llu transid %llu %s\n",
> 

Re: [PATCH 7/7] btrfs: fix mount and ioctl device scan ioctl race

[Anand Jain]

On 06/21/2018 01:51 AM, David Sterba wrote:
> Technically this extends the critical section covered by uuid_mutex to:
> 
> - parse early mount options -- here we can call device scan on paths
>    that can be passed as 'device=/dev/...'
> 
> - scan the device passed to mount
> 
> - open the devices related to the fs_devices -- this increases
>    fs_devices::opened
> 
> The race can happen when mount calls one of the scans and there's
> another one called eg. by mkfs or 'btrfs dev scan':
> 
> Mount                                  Scan
> -----                                  ----
> scan_one_device (dev1, fsid1)
>                                         scan_one_device (dev2, fsid2)
                                                            ^^^^
                                                            dev1
typo?


> 				           add the device
> 					   free stale devices
> 					       fsid1 fs_devices::opened == 0
> 					           find fsid1:dev1
> 					           free fsid1:dev1
> 					           if it's the last one,
> 					            free fs_devices of fsid1
> 						    too
> 
> open_devices (dev1, fsid1)
>     dev1 not found
> 
> When fixed, the uuid mutex will make sure that mount will increase
> fs_devices::opened and this will not be touched by the racing scan
> ioctl.

  Using uuid_mutex will unnecessarily serialize mount across different
  fsids.

  Unfortunately we don't have a test case to measure concurrency across
  btrfs fsids. When we have that, this shall fail.

  Expecting different fsids to be able to mount concurrently is a fair
  expectation. And is certainly important for large servers running
  btrfs on few luns which shall start to mount at bootup.

  These changes is kind of going in an opposite direction as I
  originally planned to improve concurrency (across fsids) by reducing
  the unnecessary uuid_mutex footprints.

  And fix the other necessaries using the fsid local atomic volume
  exclusive operations flag. Which in the long term can replace
  fs_info::BTRFS_FS_EXCL_OP as well.

  As both of these approaches fix the issue, its a trade off between the
  concerns of atomic volume exclusive operations flag (except for the
  -EBUSY part [1]) VS serialize mount across different fsids, and IMO,
  its better to make sure different fsids are concurrent in their
  scan-mount operations as it is critical to the boot-up time.

  [1]
  Though returning -EBUSY (for one of the racing mount, scan and or ready
  threads) is theoretically correct but its blunt, and it may wrongly
  categorize as regression, let me try to fix that part and ask for
  comments.

Thanks, Anand


> Reported-and-tested-by: syzbot+909a5177749d7990ffa4@syzkaller.appspotmail.com
> Reported-and-tested-by: syzbot+ceb2606025ec1cc3479c@syzkaller.appspotmail.com
 >
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/super.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 1780eb41f203..b13b871bc584 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1557,19 +1557,19 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
>   
>   	mutex_lock(&uuid_mutex);
>   	error = btrfs_parse_early_options(data, mode, fs_type, &fs_devices);
> -	mutex_unlock(&uuid_mutex);
> -	if (error)
> +	if (error) {
> +		mutex_unlock(&uuid_mutex);
>   		goto error_fs_info;
> +	}
>   
> -	mutex_lock(&uuid_mutex);
>   	error = btrfs_scan_one_device(device_name, mode, fs_type, &fs_devices);
> -	mutex_unlock(&uuid_mutex);
> -	if (error)
> +	if (error) {
> +		mutex_unlock(&uuid_mutex);
>   		goto error_fs_info;
> +	}
>   
>   	fs_info->fs_devices = fs_devices;
>   
> -	mutex_lock(&uuid_mutex);
>   	error = btrfs_open_devices(fs_devices, mode, fs_type);
>   	mutex_unlock(&uuid_mutex);
>   	if (error)
> 

Re: [PATCH 1/7] btrfs: restore uuid_mutex in btrfs_open_devices

[Anand Jain]

On 06/21/2018 01:51 AM, David Sterba wrote:
> Commit 542c5908abfe84f7b4c1 ("btrfs: replace uuid_mutex by
> device_list_mutex in btrfs_open_devices") switched to device_list_mutex
> as we need that for the device list traversal, but we also need
> uuid_mutex to protect access to fs_devices::opened to be consistent with
> other users of that item.
> 
> CC: Anand Jain <anand.jain@oracle.com>
> Signed-off-by: David Sterba <dsterba@suse.com>

  I am optimize the uuid_mutex for fsids concurrency on top these set of 
patches, so.

  Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand

> ---
>   fs/btrfs/volumes.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index e034ad9e23b4..1da162928d1a 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1146,6 +1146,7 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
>   {
>   	int ret;
>   
> +	mutex_lock(&uuid_mutex);
>   	mutex_lock(&fs_devices->device_list_mutex);
>   	if (fs_devices->opened) {
>   		fs_devices->opened++;
> @@ -1155,6 +1156,7 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
>   		ret = open_fs_devices(fs_devices, flags, holder);
>   	}
>   	mutex_unlock(&fs_devices->device_list_mutex);
> +	mutex_unlock(&uuid_mutex);
>   
>   	return ret;
>   }
> 

Re: [PATCH 2/7] btrfs: extend critical section when scanning a new device

[Anand Jain]

On 06/21/2018 01:51 AM, David Sterba wrote:
> The stale device list removal needs to be protected by device_list_mutex
> too as this could delete from the list and could race with another list
> modification and cause crash.
> 
> The device needs to be fully initialized before it's added to the list
> so the fs_devices also need to be set under the mutex.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/volumes.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 1da162928d1a..02246f9af0a3 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -791,12 +791,11 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>   		rcu_assign_pointer(device->name, name);
>   
>   		mutex_lock(&fs_devices->device_list_mutex);
> +		device->fs_devices = fs_devices;
>   		list_add_rcu(&device->dev_list, &fs_devices->devices);
>   		fs_devices->num_devices++;
> -		mutex_unlock(&fs_devices->device_list_mutex);
> -
> -		device->fs_devices = fs_devices;
>   		btrfs_free_stale_devices(path, device);


This is not correct.

btrfs_free_stale_devices need the per fs_devices local device_list_mutex
lock as it traverses through the fs_uuids list. Holding just the lock of
the %fs_devices which is being scanned or mounted is not correct.

In my workspace I have replaced this patch with ..

  4130724ec926 btrfs: fix btrfs_free_stale_devices() with needed locks
  90fedec0c200 btrfs: btrfs_free_stale_devices() rename local variables
  439eec4e943c btrfs: fix device_list_add() missing device_list_mutex()
  417ecc91e223 btrfs: do btrfs_free_stale_devices() outside of 
device_list_add()

which are in the ML as well.

Thanks, Anand


> +		mutex_unlock(&fs_devices->device_list_mutex);
>   
>   		if (disk_super->label[0])
>   			pr_info("BTRFS: device label %s devid %llu transid %llu %s\n",
> 

Re: [PATCH 3/7] btrfs: lift uuid_mutex to callers of btrfs_scan_one_device

[Anand Jain]

On 06/21/2018 01:51 AM, David Sterba wrote:
> Prepartory work to fix race between mount and device scan.
> 
> The callers will have to manage the critical section, eg. mount wants to
> scan and then call btrfs_open_devices without the ioctl scan walking in
> and modifying the fs devices in the meantime.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand

> ---
>   fs/btrfs/super.c   | 12 +++++++++++-
>   fs/btrfs/volumes.c |  4 ++--
>   2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 81107ad49f3a..735402ed3154 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -917,8 +917,10 @@ static int btrfs_parse_early_options(const char *options, fmode_t flags,
>   				error = -ENOMEM;
>   				goto out;
>   			}
> +			mutex_lock(&uuid_mutex);
>   			error = btrfs_scan_one_device(device_name,
>   					flags, holder, fs_devices);
> +			mutex_unlock(&uuid_mutex);
>   			kfree(device_name);
>   			if (error)
>   				goto out;
> @@ -1539,7 +1541,9 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
>   			return ERR_PTR(error);
>   	}
>   
> +	mutex_lock(&uuid_mutex);
>   	error = btrfs_scan_one_device(device_name, mode, fs_type, &fs_devices);
> +	mutex_unlock(&uuid_mutex);
>   	if (error)
>   		goto error_sec_opts;
>   
> @@ -2234,15 +2238,21 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
>   
>   	switch (cmd) {
>   	case BTRFS_IOC_SCAN_DEV:
> +		mutex_lock(&uuid_mutex);
>   		ret = btrfs_scan_one_device(vol->name, FMODE_READ,
>   					    &btrfs_root_fs_type, &fs_devices);
> +		mutex_unlock(&uuid_mutex);
>   		break;
>   	case BTRFS_IOC_DEVICES_READY:
> +		mutex_lock(&uuid_mutex);
>   		ret = btrfs_scan_one_device(vol->name, FMODE_READ,
>   					    &btrfs_root_fs_type, &fs_devices);
> -		if (ret)
> +		if (ret) {
> +			mutex_unlock(&uuid_mutex);
>   			break;
> +		}
>   		ret = !(fs_devices->num_devices == fs_devices->total_devices);
> +		mutex_unlock(&uuid_mutex);
>   		break;
>   	case BTRFS_IOC_GET_SUPPORTED_FEATURES:
>   		ret = btrfs_ioctl_get_supported_features((void __user*)arg);
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 02246f9af0a3..958bfe1a725c 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1226,6 +1226,8 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
>   	int ret = 0;
>   	u64 bytenr;
>   
> +	lockdep_assert_held(&uuid_mutex);
> +
>   	/*
>   	 * we would like to check all the supers, but that would make
>   	 * a btrfs mount succeed after a mkfs from a different FS.
> @@ -1244,13 +1246,11 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
>   		goto error_bdev_put;
>   	}
>   
> -	mutex_lock(&uuid_mutex);
>   	device = device_list_add(path, disk_super);
>   	if (IS_ERR(device))
>   		ret = PTR_ERR(device);
>   	else
>   		*fs_devices_ret = device->fs_devices;
> -	mutex_unlock(&uuid_mutex);
>   
>   	btrfs_release_disk_super(page);
>   
> 

Re: [PATCH 4/7] btrfs: lift uuid_mutex to callers of btrfs_open_devices

[Anand Jain]

On 06/21/2018 01:51 AM, David Sterba wrote:
> Prepartory work to fix race between mount and device scan.
> 
> The callers will have to manage the critical section, eg.  mount wants
> to scan and then call btrfs_open_devices without the ioctl scan walking
> in and modifying the fs devices in the meantime.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand

> ---
>   fs/btrfs/super.c   | 2 ++
>   fs/btrfs/volumes.c | 4 ++--
>   2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 735402ed3154..ee82d02f5453 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1569,7 +1569,9 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
>   		goto error_fs_info;
>   	}
>   
> +	mutex_lock(&uuid_mutex);
>   	error = btrfs_open_devices(fs_devices, mode, fs_type);
> +	mutex_unlock(&uuid_mutex);
>   	if (error)
>   		goto error_fs_info;
>   
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 958bfe1a725c..5336d9832ba4 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1145,7 +1145,8 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
>   {
>   	int ret;
>   
> -	mutex_lock(&uuid_mutex);
> +	lockdep_assert_held(&uuid_mutex);
> +
>   	mutex_lock(&fs_devices->device_list_mutex);
>   	if (fs_devices->opened) {
>   		fs_devices->opened++;
> @@ -1155,7 +1156,6 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
>   		ret = open_fs_devices(fs_devices, flags, holder);
>   	}
>   	mutex_unlock(&fs_devices->device_list_mutex);
> -	mutex_unlock(&uuid_mutex);
>   
>   	return ret;
>   }
> 

Re: [PATCH 5/7] btrfs: lift uuid_mutex to callers of btrfs_parse_early_options

[Anand Jain]

On 06/21/2018 01:51 AM, David Sterba wrote:
> Prepartory work to fix race between mount and device scan.
> 
> btrfs_parse_early_options calls the device scan from mount and we'll
> need to let mount completely manage the critical section.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand


> ---
>   fs/btrfs/super.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index ee82d02f5453..e3324ddf2777 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -892,6 +892,8 @@ static int btrfs_parse_early_options(const char *options, fmode_t flags,
>   	char *device_name, *opts, *orig, *p;
>   	int error = 0;
>   
> +	lockdep_assert_held(&uuid_mutex);
> +
>   	if (!options)
>   		return 0;
>   
> @@ -917,10 +919,8 @@ static int btrfs_parse_early_options(const char *options, fmode_t flags,
>   				error = -ENOMEM;
>   				goto out;
>   			}
> -			mutex_lock(&uuid_mutex);
>   			error = btrfs_scan_one_device(device_name,
>   					flags, holder, fs_devices);
> -			mutex_unlock(&uuid_mutex);
>   			kfree(device_name);
>   			if (error)
>   				goto out;
> @@ -1528,8 +1528,10 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
>   	if (!(flags & SB_RDONLY))
>   		mode |= FMODE_WRITE;
>   
> +	mutex_lock(&uuid_mutex);
>   	error = btrfs_parse_early_options(data, mode, fs_type,
>   					  &fs_devices);
> +	mutex_unlock(&uuid_mutex);
>   	if (error) {
>   		return ERR_PTR(error);
>   	}
> 

Re: [PATCH 6/7] btrfs: reorder initialization before the mount locks uuid_mutex

[Anand Jain]

On 06/21/2018 01:51 AM, David Sterba wrote:
> In preparation to take a big lock, move resource initialization before
> the critical section. It's not obvious from the diff, the desired order
> is:
> 
> - initialize mount security options
> - allocate temporary fs_info
> - allocate superblock buffers
> 
> Signed-off-by: David Sterba <dsterba@suse.com>

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand


> ---
>   fs/btrfs/super.c | 30 ++++++++++++++----------------
>   1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index e3324ddf2777..1780eb41f203 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1528,14 +1528,6 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
>   	if (!(flags & SB_RDONLY))
>   		mode |= FMODE_WRITE;
>   
> -	mutex_lock(&uuid_mutex);
> -	error = btrfs_parse_early_options(data, mode, fs_type,
> -					  &fs_devices);
> -	mutex_unlock(&uuid_mutex);
> -	if (error) {
> -		return ERR_PTR(error);
> -	}
> -
>   	security_init_mnt_opts(&new_sec_opts);
>   	if (data) {
>   		error = parse_security_options(data, &new_sec_opts);
> @@ -1543,12 +1535,6 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
>   			return ERR_PTR(error);
>   	}
>   
> -	mutex_lock(&uuid_mutex);
> -	error = btrfs_scan_one_device(device_name, mode, fs_type, &fs_devices);
> -	mutex_unlock(&uuid_mutex);
> -	if (error)
> -		goto error_sec_opts;
> -
>   	/*
>   	 * Setup a dummy root and fs_info for test/set super.  This is because
>   	 * we don't actually fill this stuff out until open_ctree, but we need
> @@ -1561,8 +1547,6 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
>   		goto error_sec_opts;
>   	}
>   
> -	fs_info->fs_devices = fs_devices;
> -
>   	fs_info->super_copy = kzalloc(BTRFS_SUPER_INFO_SIZE, GFP_KERNEL);
>   	fs_info->super_for_commit = kzalloc(BTRFS_SUPER_INFO_SIZE, GFP_KERNEL);
>   	security_init_mnt_opts(&fs_info->security_opts);
> @@ -1571,6 +1555,20 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
>   		goto error_fs_info;
>   	}
>   
> +	mutex_lock(&uuid_mutex);
> +	error = btrfs_parse_early_options(data, mode, fs_type, &fs_devices);
> +	mutex_unlock(&uuid_mutex);
> +	if (error)
> +		goto error_fs_info;
> +
> +	mutex_lock(&uuid_mutex);
> +	error = btrfs_scan_one_device(device_name, mode, fs_type, &fs_devices);
> +	mutex_unlock(&uuid_mutex);
> +	if (error)
> +		goto error_fs_info;
> +
> +	fs_info->fs_devices = fs_devices;
> +
>   	mutex_lock(&uuid_mutex);
>   	error = btrfs_open_devices(fs_devices, mode, fs_type);
>   	mutex_unlock(&uuid_mutex);
> 

Re: [PATCH 7/7] btrfs: fix mount and ioctl device scan ioctl race

[Anand Jain]

On 06/21/2018 01:51 AM, David Sterba wrote:
> Technically this extends the critical section covered by uuid_mutex to:
> 
> - parse early mount options -- here we can call device scan on paths
>    that can be passed as 'device=/dev/...'
> 
> - scan the device passed to mount
> 
> - open the devices related to the fs_devices -- this increases
>    fs_devices::opened
> 
> The race can happen when mount calls one of the scans and there's
> another one called eg. by mkfs or 'btrfs dev scan':
> 
> Mount                                  Scan
> -----                                  ----
> scan_one_device (dev1, fsid1)
>                                         scan_one_device (dev2, fsid2)
> 				           add the device
> 					   free stale devices
> 					       fsid1 fs_devices::opened == 0
> 					           find fsid1:dev1
> 					           free fsid1:dev1
> 					           if it's the last one,
> 					            free fs_devices of fsid1
> 						    too
> 
> open_devices (dev1, fsid1)
>     dev1 not found
> 
> When fixed, the uuid mutex will make sure that mount will increase
> fs_devices::opened and this will not be touched by the racing scan
> ioctl.
> 
> Reported-and-tested-by: syzbot+909a5177749d7990ffa4@syzkaller.appspotmail.com
> Reported-and-tested-by: syzbot+ceb2606025ec1cc3479c@syzkaller.appspotmail.com
> Signed-off-by: David Sterba <dsterba@suse.com>

As I said, the fsids concurrency scan/mount are coming up on top these 
patches so..

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand


> ---
>   fs/btrfs/super.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 1780eb41f203..b13b871bc584 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1557,19 +1557,19 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
>   
>   	mutex_lock(&uuid_mutex);
>   	error = btrfs_parse_early_options(data, mode, fs_type, &fs_devices);
> -	mutex_unlock(&uuid_mutex);
> -	if (error)
> +	if (error) {
> +		mutex_unlock(&uuid_mutex);
>   		goto error_fs_info;
> +	}
>   
> -	mutex_lock(&uuid_mutex);
>   	error = btrfs_scan_one_device(device_name, mode, fs_type, &fs_devices);
> -	mutex_unlock(&uuid_mutex);
> -	if (error)
> +	if (error) {
> +		mutex_unlock(&uuid_mutex);
>   		goto error_fs_info;
> +	}
>   
>   	fs_info->fs_devices = fs_devices;
>   
> -	mutex_lock(&uuid_mutex);
>   	error = btrfs_open_devices(fs_devices, mode, fs_type);
>   	mutex_unlock(&uuid_mutex);
>   	if (error)
> 

Re: [PATCH 1/7] btrfs: restore uuid_mutex in btrfs_open_devices

[David Sterba]

On Wed, Jul 04, 2018 at 04:09:50PM +0800, Anand Jain wrote:
> On 06/21/2018 01:51 AM, David Sterba wrote:
> > Commit 542c5908abfe84f7b4c1 ("btrfs: replace uuid_mutex by
> > device_list_mutex in btrfs_open_devices") switched to device_list_mutex
> > as we need that for the device list traversal, but we also need
> > uuid_mutex to protect access to fs_devices::opened to be consistent with
> > other users of that item.
> > 
> > CC: Anand Jain <anand.jain@oracle.com>
> > Signed-off-by: David Sterba <dsterba@suse.com>
> 
>   I am optimize the uuid_mutex for fsids concurrency on top these set of 
> patches, so.
> 
>   Reviewed-by: Anand Jain <anand.jain@oracle.com>

So this should go to 4.18, instead of reverting 542c5908abfe8.

Re: [PATCH 2/7] btrfs: extend critical section when scanning a new device

[David Sterba]

On Wed, Jul 04, 2018 at 04:18:09PM +0800, Anand Jain wrote:
> 
> 
> On 06/21/2018 01:51 AM, David Sterba wrote:
> > The stale device list removal needs to be protected by device_list_mutex
> > too as this could delete from the list and could race with another list
> > modification and cause crash.
> > 
> > The device needs to be fully initialized before it's added to the list
> > so the fs_devices also need to be set under the mutex.
> > 
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> >   fs/btrfs/volumes.c | 5 ++---
> >   1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index 1da162928d1a..02246f9af0a3 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -791,12 +791,11 @@ static noinline struct btrfs_device *device_list_add(const char *path,
> >   		rcu_assign_pointer(device->name, name);
> >   
> >   		mutex_lock(&fs_devices->device_list_mutex);
> > +		device->fs_devices = fs_devices;
> >   		list_add_rcu(&device->dev_list, &fs_devices->devices);
> >   		fs_devices->num_devices++;
> > -		mutex_unlock(&fs_devices->device_list_mutex);
> > -
> > -		device->fs_devices = fs_devices;
> >   		btrfs_free_stale_devices(path, device);
> 
> 
> This is not correct.
> 
> btrfs_free_stale_devices need the per fs_devices local device_list_mutex
> lock as it traverses through the fs_uuids list. Holding just the lock of
> the %fs_devices which is being scanned or mounted is not correct.

I see, ok. The use of fs_devs is not very visible in
btrfs_free_stale_devices but at least the function comment hints that
all devices are being traversed.