Re: [PATCH v4 6/6] btrfs: use btrfs_get_dev_args_from_path in dev removal ioctls

On 06/10/2021 17:05, Nikolay Borisov wrote:

On 6.10.21 г. 11:54, Anand Jain wrote:
<snip>

quoted

quoted

Suggested-by: Anand Jain <redacted>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
   fs/btrfs/ioctl.c   | 67 +++++++++++++++++++++++++++-------------------
   fs/btrfs/volumes.c | 15 +++++------
   fs/btrfs/volumes.h |  2 +-
   3 files changed, 48 insertions(+), 36 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index b8508af4e539..c9d3f375df83 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c

@@ -3160,6 +3160,7 @@ static long btrfs_ioctl_add_dev(struct

btrfs_fs_info *fs_info, void __user *arg)
     static long btrfs_ioctl_rm_dev_v2(struct file *file, void __user
*arg)
   {
+    BTRFS_DEV_LOOKUP_ARGS(args);
       struct inode *inode = file_inode(file);
       struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
       struct btrfs_ioctl_vol_args_v2 *vol_args;

@@ -3171,35 +3172,39 @@ static long btrfs_ioctl_rm_dev_v2(struct file

*file, void __user *arg)
       if (!capable(CAP_SYS_ADMIN))
           return -EPERM;
   -    ret = mnt_want_write_file(file);
-    if (ret)
-        return ret;
-
       vol_args = memdup_user(arg, sizeof(*vol_args));
       if (IS_ERR(vol_args)) {

quoted

           ret = PTR_ERR(vol_args);
-        goto err_drop;
+        goto out;

      return  PTR_ERR(vol_args);

is fine here.

quoted

       }
         if (vol_args->flags & ~BTRFS_DEVICE_REMOVE_ARGS_MASK) {
           ret = -EOPNOTSUPP;
           goto out;

At the label out, we call, btrfs_put_dev_args_from_path(&args).
However, the args.fsid and args.uuid might not have initialized
to NULL or a valid kmem address.

On tha contrary, the fact that args is initialized via a designated
initializer guarantees that the rest of the members of the struct are
going to be zero initialized, or more precisley as if they had a "static
lifetime"

https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html

<snip>

Sorry for the late reply, I see the patch made it to the misc-next.
I missed both this email and the point that the rest of the members
initialize to zero. Yep. Looks good.

Reviewed-by: Anand Jain <redacted>

Thanks, Anand