[PATCH 0/4] Misc cleanups around device addition

[PATCH 0/4] Misc cleanups around device addition

[Nikolay Borisov]

Here are 4 minor cleanups around btrfs_init_new_device. First one converts
a readonly usage of device_lists to using RCU. Second patch slightly simplifies
the logic around locking when seed device is used. Third one removes a leftover
from rewrite of btrfs_free_stale_devices. And the final one replaces an
opencoded filemap_write_and_wait on the bdev inode with the more consistent
sync_blockdev.

All these have passed xfstest and don't constitute functional changes.

Nikolay Borisov (4):
  btrfs: Use rcu when iterating devices in btrfs_init_new_device
  btrfs: Refactor locked condition in btrfs_init_new_device
  btrfs: Remove redundant code from btrfs_free_stale_devices
  btrfs: Don't opencode sync_blockdev

 fs/btrfs/volumes.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

--
2.17.1

[PATCH 1/4] btrfs: Use rcu when iterating devices in btrfs_init_new_device

[Nikolay Borisov]

When adding a new device there's a mandatory check to see if a device is
being duplicated to the filesystem it's added to. Since this is a
read-only operations not necessary to take device_list_mutex and can simply
make do with an rcu-readlock. No semantics changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/volumes.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9f338d9db51b..0795ab511f8b 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2502,16 +2502,15 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 
 	filemap_write_and_wait(bdev->bd_inode->i_mapping);
 
-	mutex_lock(&fs_devices->device_list_mutex);
-	list_for_each_entry(device, &fs_devices->devices, dev_list) {
+	rcu_read_lock();
+	list_for_each_entry_rcu(device, &fs_devices->devices, dev_list) {
 		if (device->bdev == bdev) {
 			ret = -EEXIST;
-			mutex_unlock(
-				&fs_devices->device_list_mutex);
+			rcu_read_unlock();
 			goto error;
 		}
 	}
-	mutex_unlock(&fs_devices->device_list_mutex);
+	rcu_read_unlock();
 
 	device = btrfs_alloc_device(fs_info, NULL, NULL);
 	if (IS_ERR(device)) {
-- 
2.17.1

Re: [PATCH 1/4] btrfs: Use rcu when iterating devices in btrfs_init_new_device

[Johannes Thumshirn]

On 22/07/2020 10:09, Nikolay Borisov wrote:
> When adding a new device there's a mandatory check to see if a device is
> being duplicated to the filesystem it's added to. Since this is a
> read-only operations not necessary to take device_list_mutex and can simply
> make do with an rcu-readlock. No semantics changes.

Hmm what are the actual locking rules for the device list? For instance looking
at add_missing_dev() in volumes.c addition to the list is unprotected (both from
read_one_chunk() and read_one_dev()). Others (i.e. btrfs_init_new_device()) do
a list_add_rcu(). clone_fs_devices() takes the device_list_mutex and so on.

Thanks,
	Johannes

Re: [PATCH 1/4] btrfs: Use rcu when iterating devices in btrfs_init_new_device

[Nikolay Borisov]

On 22.07.20 г. 12:17 ч., Johannes Thumshirn wrote:
> On 22/07/2020 10:09, Nikolay Borisov wrote:
>> When adding a new device there's a mandatory check to see if a device is
>> being duplicated to the filesystem it's added to. Since this is a
>> read-only operations not necessary to take device_list_mutex and can simply
>> make do with an rcu-readlock. No semantics changes.
> 
> Hmm what are the actual locking rules for the device list? For instance looking
> at add_missing_dev() in volumes.c addition to the list is unprotected (both from
> read_one_chunk() and read_one_dev()). Others (i.e. btrfs_init_new_device()) do
> a list_add_rcu(). clone_fs_devices() takes the device_list_mutex and so on.

Bummer, the locking rules are supposed to be those documented atop
volume.c, namely:

 * fs_devices::device_list_mutex (per-fs, with RCU)

    18  * ------------------------------------------------

    17  * protects updates to fs_devices::devices, ie. adding and
deleting
    16  *

    15  * simple list traversal with read-only actions can be done with
RCU protection
    14  *

    13  * may be used to exclude some operations from running
concurrently without any
    12  * modifications to the list (see write_all_supers)



However, your observations seem correct and rather annoying. Let me go
and try and figure this out...

> 
> Thanks,
> 	Johannes
> 

Re: [PATCH 1/4] btrfs: Use rcu when iterating devices in btrfs_init_new_device

[David Sterba]

On Wed, Jul 22, 2020 at 01:36:15PM +0300, Nikolay Borisov wrote:
> 
> 
> On 22.07.20 г. 12:17 ч., Johannes Thumshirn wrote:
> > On 22/07/2020 10:09, Nikolay Borisov wrote:
> >> When adding a new device there's a mandatory check to see if a device is
> >> being duplicated to the filesystem it's added to. Since this is a
> >> read-only operations not necessary to take device_list_mutex and can simply
> >> make do with an rcu-readlock. No semantics changes.
> > 
> > Hmm what are the actual locking rules for the device list? For instance looking
> > at add_missing_dev() in volumes.c addition to the list is unprotected (both from
> > read_one_chunk() and read_one_dev()). Others (i.e. btrfs_init_new_device()) do
> > a list_add_rcu(). clone_fs_devices() takes the device_list_mutex and so on.
> 
> Bummer, the locking rules are supposed to be those documented atop
> volume.c, namely:
> 
>  * fs_devices::device_list_mutex (per-fs, with RCU)
> 
>     18  * ------------------------------------------------
> 
>     17  * protects updates to fs_devices::devices, ie. adding and
> deleting
>     16  *
> 
>     15  * simple list traversal with read-only actions can be done with
> RCU protection
>     14  *
> 
>     13  * may be used to exclude some operations from running
> concurrently without any
>     12  * modifications to the list (see write_all_supers)
> 
> 
> 
> However, your observations seem correct and rather annoying. Let me go
> and try and figure this out...

For device list it's important to know from which context is the list
used.

On unmoutned filesystems, the devices can be added by scanning ioctl.

On mounted filesystem, before the mount is finished, the devices cannot
be concurrently used (single-threaded context) and uuid_mutex is
temporarily protecting the devices agains scan ioctl.

On finished mount device list must be protected by device_list_mutex
from the same filesystem changes (ioctls, superblock write). Device
scanning is a no-op here, but still needs to use the uuid_mutex.

Enter RCU. For performance reasons we don't want to take
device_list_mutex for read-only operations like show_devname or lookups
of device id, where it's not critical that the returned information is
stale.

The mentioned helpers are used by various functions and the context is
not obvious or documented, but it should be clear once the caller chain
is known.

I can turn that into comments but please let me know if this is
sufficient as explanation or if you need more.

Re: [PATCH 1/4] btrfs: Use rcu when iterating devices in btrfs_init_new_device

[Johannes Thumshirn]

On 22/07/2020 16:48, David Sterba wrote:
> On Wed, Jul 22, 2020 at 01:36:15PM +0300, Nikolay Borisov wrote:
>>
>>
>> On 22.07.20 г. 12:17 ч., Johannes Thumshirn wrote:
>>> On 22/07/2020 10:09, Nikolay Borisov wrote:
>>>> When adding a new device there's a mandatory check to see if a device is
>>>> being duplicated to the filesystem it's added to. Since this is a
>>>> read-only operations not necessary to take device_list_mutex and can simply
>>>> make do with an rcu-readlock. No semantics changes.
>>>
>>> Hmm what are the actual locking rules for the device list? For instance looking
>>> at add_missing_dev() in volumes.c addition to the list is unprotected (both from
>>> read_one_chunk() and read_one_dev()). Others (i.e. btrfs_init_new_device()) do
>>> a list_add_rcu(). clone_fs_devices() takes the device_list_mutex and so on.
>>
>> Bummer, the locking rules are supposed to be those documented atop
>> volume.c, namely:
>>
>>  * fs_devices::device_list_mutex (per-fs, with RCU)
>>
>>     18  * ------------------------------------------------
>>
>>     17  * protects updates to fs_devices::devices, ie. adding and
>> deleting
>>     16  *
>>
>>     15  * simple list traversal with read-only actions can be done with
>> RCU protection
>>     14  *
>>
>>     13  * may be used to exclude some operations from running
>> concurrently without any
>>     12  * modifications to the list (see write_all_supers)
>>
>>
>>
>> However, your observations seem correct and rather annoying. Let me go
>> and try and figure this out...
> 
> For device list it's important to know from which context is the list
> used.
> 
> On unmoutned filesystems, the devices can be added by scanning ioctl.
> 
> On mounted filesystem, before the mount is finished, the devices cannot
> be concurrently used (single-threaded context) and uuid_mutex is
> temporarily protecting the devices agains scan ioctl.
> 
> On finished mount device list must be protected by device_list_mutex
> from the same filesystem changes (ioctls, superblock write). Device
> scanning is a no-op here, but still needs to use the uuid_mutex.
> 
> Enter RCU. For performance reasons we don't want to take
> device_list_mutex for read-only operations like show_devname or lookups
> of device id, where it's not critical that the returned information is
> stale.

OK I got confused by a) the mix of unlocked, device_list_mutex and 
uuid_mutex use and b) sometimes we use plain list_* operations and
sometimes rcu list ones.

 
> The mentioned helpers are used by various functions and the context is
> not obvious or documented, but it should be clear once the caller chain
> is known.
> 
> I can turn that into comments but please let me know if this is
> sufficient as explanation or if you need more.
> 

This would be great yes.

Re: [PATCH 1/4] btrfs: Use rcu when iterating devices in btrfs_init_new_device

[Nikolay Borisov]

On 22.07.20 г. 17:47 ч., David Sterba wrote:
> On Wed, Jul 22, 2020 at 01:36:15PM +0300, Nikolay Borisov wrote:
>>
>>
>> On 22.07.20 г. 12:17 ч., Johannes Thumshirn wrote:
>>> On 22/07/2020 10:09, Nikolay Borisov wrote:
>>>> When adding a new device there's a mandatory check to see if a device is
>>>> being duplicated to the filesystem it's added to. Since this is a
>>>> read-only operations not necessary to take device_list_mutex and can simply
>>>> make do with an rcu-readlock. No semantics changes.
>>>
>>> Hmm what are the actual locking rules for the device list? For instance looking
>>> at add_missing_dev() in volumes.c addition to the list is unprotected (both from
>>> read_one_chunk() and read_one_dev()). Others (i.e. btrfs_init_new_device()) do
>>> a list_add_rcu(). clone_fs_devices() takes the device_list_mutex and so on.
>>
>> Bummer, the locking rules are supposed to be those documented atop
>> volume.c, namely:
>>
>>  * fs_devices::device_list_mutex (per-fs, with RCU)
>>
>>     18  * ------------------------------------------------
>>
>>     17  * protects updates to fs_devices::devices, ie. adding and
>> deleting
>>     16  *
>>
>>     15  * simple list traversal with read-only actions can be done with
>> RCU protection
>>     14  *
>>
>>     13  * may be used to exclude some operations from running
>> concurrently without any
>>     12  * modifications to the list (see write_all_supers)
>>
>>
>>
>> However, your observations seem correct and rather annoying. Let me go
>> and try and figure this out...
> 
> For device list it's important to know from which context is the list
> used.
> 
> On unmoutned filesystems, the devices can be added by scanning ioctl.
> 
> On mounted filesystem, before the mount is finished, the devices cannot
> be concurrently used (single-threaded context) and uuid_mutex is
> temporarily protecting the devices agains scan ioctl.
> 
> On finished mount device list must be protected by device_list_mutex
> from the same filesystem changes (ioctls, superblock write). Device
> scanning is a no-op here, but still needs to use the uuid_mutex.
> 
> Enter RCU. For performance reasons we don't want to take
> device_list_mutex for read-only operations like show_devname or lookups
> of device id, where it's not critical that the returned information is
> stale.
> 
> The mentioned helpers are used by various functions and the context is
> not obvious or documented, but it should be clear once the caller chain
> is known.
> 
> I can turn that into comments but please let me know if this is
> sufficient as explanation or if you need more.
> 

Yes having those different contexts spelled out is really beneficial.

[PATCH 2/4] btrfs: Refactor locked condition in btrfs_init_new_device

[Nikolay Borisov]

Invert unlocked to locked and exploit the fact it can only ever be
modified if we are adding a new device to a seed filesystem. This allows
to simplify the check in error: label. No semantics changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/volumes.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0795ab511f8b..384614fe0e2a 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2484,7 +2484,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	u64 orig_super_num_devices;
 	int seeding_dev = 0;
 	int ret = 0;
-	bool unlocked = false;
+	bool locked = false;
 
 	if (sb_rdonly(sb) && !fs_devices->seeding)
 		return -EROFS;
@@ -2498,6 +2498,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 		seeding_dev = 1;
 		down_write(&sb->s_umount);
 		mutex_lock(&uuid_mutex);
+		locked = true;
 	}
 
 	filemap_write_and_wait(bdev->bd_inode->i_mapping);
@@ -2629,7 +2630,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	if (seeding_dev) {
 		mutex_unlock(&uuid_mutex);
 		up_write(&sb->s_umount);
-		unlocked = true;
+		locked = false;
 
 		if (ret) /* transaction commit */
 			return ret;
@@ -2690,7 +2691,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	btrfs_free_device(device);
 error:
 	blkdev_put(bdev, FMODE_EXCL);
-	if (seeding_dev && !unlocked) {
+	if (locked) {
 		mutex_unlock(&uuid_mutex);
 		up_write(&sb->s_umount);
 	}
-- 
2.17.1

Re: [PATCH 2/4] btrfs: Refactor locked condition in btrfs_init_new_device

[Anand Jain]

On 22/7/20 4:09 pm, Nikolay Borisov wrote:
> Invert unlocked to locked and exploit the fact it can only ever be
> modified if we are adding a new device to a seed filesystem. This allows
> to simplify the check in error: label. No semantics changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

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

[PATCH 3/4] btrfs: Remove redundant code from btrfs_free_stale_devices

[Nikolay Borisov]

Following the refactor of btrfs_free_stale_devices in
7bcb8164ad94 ("btrfs: use device_list_mutex when removing stale devices")
fs_devices are freed after they have been iterated by the inner
list_for_each so the UAF fixed by introducing the break in
fd649f10c3d2 ("btrfs: Fix use-after-free when cleaning up fs_devs with
a single stale device") is no longer necessary. Just remove it
altogether. No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/volumes.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 384614fe0e2a..17047c118969 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -588,8 +588,6 @@ static int btrfs_free_stale_devices(const char *path,
 			btrfs_free_device(device);
 
 			ret = 0;
-			if (fs_devices->num_devices == 0)
-				break;
 		}
 		mutex_unlock(&fs_devices->device_list_mutex);
 
-- 
2.17.1

Re: [PATCH 3/4] btrfs: Remove redundant code from btrfs_free_stale_devices

[Anand Jain]

On 22/7/20 4:09 pm, Nikolay Borisov wrote:
> Following the refactor of btrfs_free_stale_devices in
> 7bcb8164ad94 ("btrfs: use device_list_mutex when removing stale devices")
> fs_devices are freed after they have been iterated by the inner
> list_for_each so the UAF fixed by introducing the break in
> fd649f10c3d2 ("btrfs: Fix use-after-free when cleaning up fs_devs with
> a single stale device") is no longer necessary. Just remove it
> altogether. No functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>   fs/btrfs/volumes.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 384614fe0e2a..17047c118969 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -588,8 +588,6 @@ static int btrfs_free_stale_devices(const char *path,
>   			btrfs_free_device(device);
>   
>   			ret = 0;
> -			if (fs_devices->num_devices == 0)
> -				break;
>   		}
>   		mutex_unlock(&fs_devices->device_list_mutex);
>   
> 


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

Thanks, Anand

[PATCH 4/4] btrfs: Don't opencode sync_blockdev

[Nikolay Borisov]

Instead of opencoding filemap_write_and_wait simply call syncblockdev as
it makes it abundandtly clear what's going on and why this is used. No
semantics changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/volumes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 17047c118969..d42571f36fcf 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2499,7 +2499,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 		locked = true;
 	}
 
-	filemap_write_and_wait(bdev->bd_inode->i_mapping);
+	sync_blockdev(bdev);
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(device, &fs_devices->devices, dev_list) {
-- 
2.17.1

Re: [PATCH 4/4] btrfs: Don't opencode sync_blockdev

[Johannes Thumshirn]

On 22/07/2020 10:09, Nikolay Borisov wrote:
> Instead of opencoding filemap_write_and_wait simply call syncblockdev as
                                             sync_blockdev() ~^
> it makes it abundandtly clear what's going on and why this is used. No
  abundantly ~^

Otherwise:
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 4/4] btrfs: Don't opencode sync_blockdev

[Anand Jain]

On 22/7/20 4:09 pm, Nikolay Borisov wrote:
> Instead of opencoding filemap_write_and_wait simply call syncblockdev as
> it makes it abundandtly clear what's going on and why this is used. No
> semantics changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>   fs/btrfs/volumes.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 17047c118969..d42571f36fcf 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2499,7 +2499,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>   		locked = true;
>   	}
>   
> -	filemap_write_and_wait(bdev->bd_inode->i_mapping);
> +	sync_blockdev(bdev);

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

Thanks, Anand

>   
>   	rcu_read_lock();
>   	list_for_each_entry_rcu(device, &fs_devices->devices, dev_list) {
> 

Re: [PATCH 0/4] Misc cleanups around device addition

[Nikolay Borisov]

On 22.07.20 г. 11:09 ч., Nikolay Borisov wrote:
> Here are 4 minor cleanups around btrfs_init_new_device. First one converts
> a readonly usage of device_lists to using RCU. Second patch slightly simplifies
> the logic around locking when seed device is used. Third one removes a leftover
> from rewrite of btrfs_free_stale_devices. And the final one replaces an
> opencoded filemap_write_and_wait on the bdev inode with the more consistent
> sync_blockdev.
> 
> All these have passed xfstest and don't constitute functional changes.
> 
> Nikolay Borisov (4):
>   btrfs: Use rcu when iterating devices in btrfs_init_new_device
>   btrfs: Refactor locked condition in btrfs_init_new_device
>   btrfs: Remove redundant code from btrfs_free_stale_devices
>   btrfs: Don't opencode sync_blockdev
> 
>  fs/btrfs/volumes.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> --
> 2.17.1
> 

Ping

Re: [PATCH 0/4] Misc cleanups around device addition

[David Sterba]

On Wed, Jul 22, 2020 at 11:09:21AM +0300, Nikolay Borisov wrote:
> Here are 4 minor cleanups around btrfs_init_new_device. First one converts
> a readonly usage of device_lists to using RCU. Second patch slightly simplifies
> the logic around locking when seed device is used. Third one removes a leftover
> from rewrite of btrfs_free_stale_devices. And the final one replaces an
> opencoded filemap_write_and_wait on the bdev inode with the more consistent
> sync_blockdev.
> 
> All these have passed xfstest and don't constitute functional changes.
> 
> Nikolay Borisov (4):
>   btrfs: Use rcu when iterating devices in btrfs_init_new_device
>   btrfs: Refactor locked condition in btrfs_init_new_device
>   btrfs: Remove redundant code from btrfs_free_stale_devices
>   btrfs: Don't opencode sync_blockdev

Added to for-next, thanks. I've updated patch 1 with explanation why RCU
is safe.