Fix locking when scanning devices

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(-)

Quoted 26 lines · 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

Quoted 4 lines

 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).

From what I've seen, there's not much potential for significant changes.

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 <[email protected]>

---
  fs/btrfs/volumes.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)

fs/btrfs/volumes.c +2 -3

index 1da162928d1a..02246f9af0a3 100644
@@ -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",

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 [email protected]

Reported-and-tested-by [email protected]

Signed-off-by David Sterba <[email protected]>

---
  fs/btrfs/super.c | 12 ++++++------
  1 file changed, 6 insertions(+), 6 deletions(-)

fs/btrfs/super.c +6 -6

index 1780eb41f203..b13b871bc584 100644
@@ -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)

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 <[email protected]>

Signed-off-by David Sterba <[email protected]>

---
  fs/btrfs/volumes.c | 2 ++
  1 file changed, 2 insertions(+)

fs/btrfs/volumes.c +2 -0

index e034ad9e23b4..1da162928d1a 100644
@@ -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;
  }

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 <[email protected]>

---
  fs/btrfs/volumes.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)

fs/btrfs/volumes.c +0 -0

index 1da162928d1a..02246f9af0a3 100644

fs/btrfs/volumes.c +1 -0

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

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 <[email protected]>

---
  fs/btrfs/super.c   | 12 +++++++++++-
  fs/btrfs/volumes.c |  4 ++--
  2 files changed, 13 insertions(+), 3 deletions(-)

fs/btrfs/super.c +11 -1

index 81107ad49f3a..735402ed3154 100644
@@ -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);

fs/btrfs/volumes.c +2 -2

index 02246f9af0a3..958bfe1a725c 100644
@@ -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);
 

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 <[email protected]>

---
  fs/btrfs/super.c   | 2 ++
  fs/btrfs/volumes.c | 4 ++--
  2 files changed, 4 insertions(+), 2 deletions(-)

fs/btrfs/super.c +2 -0

index 735402ed3154..ee82d02f5453 100644
@@ -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;
 

fs/btrfs/volumes.c +2 -2

index 958bfe1a725c..5336d9832ba4 100644
@@ -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;
  }

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 <[email protected]>

---
  fs/btrfs/super.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

fs/btrfs/super.c +4 -2

index ee82d02f5453..e3324ddf2777 100644
@@ -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);
  	}

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 <[email protected]>

---
  fs/btrfs/super.c | 30 ++++++++++++++----------------
  1 file changed, 14 insertions(+), 16 deletions(-)

fs/btrfs/super.c +14 -16

index e3324ddf2777..1780eb41f203 100644
@@ -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);

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 [email protected]

Reported-and-tested-by [email protected]

Signed-off-by David Sterba <[email protected]>

---
  fs/btrfs/super.c | 12 ++++++------
  1 file changed, 6 insertions(+), 6 deletions(-)

fs/btrfs/super.c +6 -6

index 1780eb41f203..b13b871bc584 100644
@@ -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)

Quoted 6 lines · 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.

Quoted 2 lines

CC Anand Jain <[email protected]>

Signed-off-by David Sterba <[email protected]>

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

Reviewed-by Anand Jain <[email protected]>

Quoted 17 lines · 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 <[email protected]>

---
  fs/btrfs/volumes.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)

fs/btrfs/volumes.c +0 -0

index 1da162928d1a..02246f9af0a3 100644

fs/btrfs/volumes.c static noinline struct btrfs_device *device_list_add(const char *path, · line 797

Quoted 10 lines

fs/btrfs/volumes.c +1 -3

@@ -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.