Re: [PATCH v2 3/7] btrfs: handle allocation failure in strdup

[Nikolay Borisov <nborisov@xxxxxxxx>]

On 13.11.19 г. 12:27 ч., Johannes Thumshirn wrote:
> Gracefully handle allocation failures in btrfs_close_one_device()'s
> rcu_string_strdup() instead of crashing the machine.
> 
> Signed-off-by: Johannes Thumshirn <jthumshirn@xxxxxxx>
> ---
>  fs/btrfs/volumes.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 0a2a73907563..e5864ca3bb3b 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1064,7 +1064,7 @@ static void btrfs_close_bdev(struct btrfs_device *device)
>  static int btrfs_close_one_device(struct btrfs_device *device)
>  {
>  	struct btrfs_fs_devices *fs_devices = device->fs_devices;
> -	struct btrfs_device *new_device;
> +	struct btrfs_device *new_device = NULL;
>  	struct rcu_string *name;
>  
>  	new_device = btrfs_alloc_device(NULL, &device->devid,
> @@ -1072,6 +1072,15 @@ static int btrfs_close_one_device(struct btrfs_device *device)
>  	if (IS_ERR(new_device))
>  		goto err_close_device;
>  
> +	/* Safe because we are under uuid_mutex */
> +	if (device->name) {
> +		name = rcu_string_strdup(device->name->str, GFP_NOFS);
> +		if (!name)
> +			goto err_free_device;
> +
> +		rcu_assign_pointer(new_device->name, name);
> +	}

This could really be:


diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e148b13905c5..7bb3cd8afa7a 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1086,11 +1086,8 @@ static void btrfs_close_one_device(struct
btrfs_device *device)
        BUG_ON(IS_ERR(new_device)); /* -ENOMEM */

        /* Safe because we are under uuid_mutex */
-       if (device->name) {
-               name = rcu_string_strdup(device->name->str, GFP_NOFS);
-               BUG_ON(!name); /* -ENOMEM */
-               rcu_assign_pointer(new_device->name, name);
-       }
+       new_device->name = device->name;
+       device->name = NULL;

        list_replace_rcu(&device->dev_list, &new_device->dev_list);
        new_device->fs_devices = device->fs_devices;

rcu_string_free already checks if device->name is non-NULL.


<snip>