Re: [PATCH] linux-user: Add support for a group of '_V2' btrfs ioctls

[Laurent Vivier]

Le 23/08/2020 à 21:59, Filip Bozuta a écrit :
> This patch introduces functionality for following btrfs ioctls:
> 
> BTRFS_IOC_SUBVOL_CREATE_V2 - Adding a new btrfs subvolume
> 
>     Create a new btrfs subvolume (same as with BTRFS_IOC_SUBVOL_CREATE).
>     The third ioctl's argument is a pointer to a following type:
> 
>     struct btrfs_ioctl_vol_args_v2 {
>       __s64 fd;
>       __u64 transid;
>       __u64 flags;
>       union {
>               struct {
>                       __u64 size;
>                       struct btrfs_qgroup_inherit __user *qgroup_inherit;
>               };
>               __u64 unused[4];
>       };
>       union {
>               char name[BTRFS_SUBVOL_NAME_MAX + 1];
>               __u64 devid;
>               __u64 subvolid; /* added in kernel version 5.8 */
>       };
>     };
> 
>     When calling this ioctl, the 'name' field should be filled with
>     the aproppriate value that contains the name of the subvolume that
>     is to be created. The flags field can take values that are
>     'BTRFS_SUBVOL_RDONLY' or 'BTRFS_SUBVOL_QGROUP_INHERIT'. If the
>     latter is specified, the field 'qgroup_inherit' should be filled
>     with aproppriate values of the quota group in which the newly
>     created subvolume is to be added. The definition of
>     'struct btrfs_qgroup_inherit' can be found at:
>     
> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/btrfs.h#L90
> 
> BTRFS_IOC_SNAP_CREATE_V2 - Adding a btrfs snapshot for a subvolume
> 
>     Create a new btrfs snapshot for a specified subvolume (same as with
>     BTRFS_IOC_SNAP_CREATE). The ioctl's third argument is a pointer to
>     the above mentioned 'struct btrfs_ioctl_vol_args_v2'. Before calling
>     this ioctl, field 'fd' should be filled with the aproppriate file
>     descriptor value for the btrfs subvolume for which the snapshot is
>     to be created. Also, the 'name' field should be filled with the
>     aproppriate value that represents the name of the snapshot that is
>     to be created. The 'flags' field takes the same values as in case
>     of 'BTRFS_IOC_SUBVOL_CREATE_V2' and represents the same functionality.
> 
> BTRFS_IOC_RM_DEV_V2 - Removing a btrfs device
> 
>     Delete a btrfs device (same as with BTRFS_IOC_RM_DEV). The ioctl's third
>     argument is a pointer to the above mentioned 'struct 
> btrfs_ioctl_vol_args_v2'.
>     Before calling this ioctl, either the 'name' or the 'devid' field should
>     be filled with the name or id of the device that is to be removed. Also, 
> the
>     flags field should be filled either with 0 or 'BTRFS_DEVICE_SPEC_BY_ID'
>     depending on if the device is gonna be specified via name or id.
> 
> BTRFS_IOC_SNAP_DESTROY_V2 - Removing a btrfs snapshot
> 
>     Remove a btrfs snapshot (same as with BTRFS_IOC_SNAP_DESTROY). The ioctl's
>     third argument is a pointer to the above mentioned 'struct 
> btrfs_ioctl_vol_args_v2'.
>     Before calling this ioctl, either the 'name' or the 'subvolid' field 
> should
>     be filled with the name or id of the snapshot that is to be removed. 
> Also, the
>     flags field should be filled either with 0 or 'BTRFS_SUBVOL_SPEC_BY_ID'
>     depending on if the snapshot is gonna be specified via name or id.
> 
> Implementation notes:
> 
>     Since the third argument of the implemented ioctl's is a
>     structure that contains unions, a special converting function
>     'target_to_host_btrfs_ioctl_vol_args_v2' was defined in 'syscall.c'.
>     This function is called instead of 'thunk_convert()' to convert
>     the values of the third argument from target to host. All of
>     the ioctls in this patch are of type 'IOW' which is why a converting
>     function from host to target is not required. Also, a separate printing
>     function was defined in file 'strace.c' that is called instead of
>     'thunk_print()' to print the contents of the third argument.
> 
> Signed-off-by: Filip Bozuta <Filip.Bozuta@syrmia.com>
> Based-on: <20200811164553.27713-2-Filip.Bozuta@syrmia.com>
> Based-on: <20200723210233.349690-4-Filip.Bozuta@syrmia.com>
> ---
>  linux-user/ioctls.h        | 16 +++++++
>  linux-user/qemu.h          |  5 ++
>  linux-user/strace.c        | 95 ++++++++++++++++++++++++++++++++++++++
>  linux-user/syscall.c       | 77 ++++++++++++++++++++++++++++++
>  linux-user/syscall_defs.h  | 28 +++++++++++
>  linux-user/syscall_types.h |  5 ++
>  thunk.c                    |  2 +-
>  7 files changed, 227 insertions(+), 1 deletion(-)
> 

> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index a69a0bd347..4add164673 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -708,6 +708,11 @@ static inline uint64_t target_offset64(uint64_t word0, 
> uint64_t word1)
>  
>  void print_termios(void *arg);
>  
> +#if defined(BTRFS_IOC_SUBVOL_CREATE_V2) || defined(BTRFS_IOC_SNAP_CREATE_V2) 
> \
> + || defined(BTRFS_IOC_SNAP_DESTROY_V2)  || defined(BTRFS_IOC_RM_DEV_V2)

The #if is not needed on the declaration of the symbol. It can be unused
or not defined, there will be no error.

> +void print_btrfs_ioctl_vol_args_v2(void *arg);
> +#endif
> +
>  /* ARM EABI and MIPS expect 64bit types aligned even on pairs or registers */
>  #ifdef TARGET_ARM
>  static inline int regpairs_aligned(void *cpu_env, int num)
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index b9ba39ce6e..04fe38a846 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -9,6 +9,9 @@
>  #include <netinet/tcp.h>
>  #include <linux/if_packet.h>
>  #include <linux/netlink.h>
> +#ifdef CONFIG_BTRFS
> +#include <linux/btrfs.h>
> +#endif
>  #include <sched.h>
>  #include "qemu.h"
>  
...
> @@ -1774,6 +1793,82 @@ print_termios(void *arg)
>      qemu_log("}");
>  }
>  
> +#if defined(BTRFS_IOC_SUBVOL_CREATE_V2) || defined(BTRFS_IOC_SNAP_CREATE_V2) 
> \
> + || defined(BTRFS_IOC_SNAP_DESTROY_V2)  || defined(BTRFS_IOC_RM_DEV_V2)
> +void
> +print_btrfs_ioctl_vol_args_v2(void *arg)
> +{
> +    struct target_btrfs_ioctl_vol_args_v2 *target_args_v2 = arg;
> +    uint64_t flags = tswap64(target_args_v2->flags);
> +    int device_spec_by_id = 0;
> +    int subvol_spec_by_id = 0;
> +
> +    qemu_log("{fd = %" PRId64 ", flags = ", tswap64(target_args_v2->fd));
> +    print_flags(btrfs_args_vol_v2_flags, flags, 0);
> +    qemu_log(" transid = %" PRIu64 ",", tswap64(target_args_v2->transid));
> +
> +#ifdef BTRFS_SUBVOL_QGROUP_INHERIT
> +    if (flags & BTRFS_SUBVOL_QGROUP_INHERIT) {
> +
> +        struct btrfs_qgroup_inherit *target_inherit;
> +        abi_long inherit_addr = tswapal(target_args_v2->qgroup_inherit);
> +
> +        target_inherit = lock_user(VERIFY_READ, inherit_addr,
> +                                   sizeof(*target_inherit), 1);
> +
> +        if (target_inherit) {
> +            qemu_log(" size = %" PRIu64,
> +                     tswap64(target_args_v2->size));
> +            qemu_log(", qgroup_inherit = {");
> +
> +            qemu_log("flags = %" PRIu64,
> +                     tswap64(target_inherit->flags));
> +            qemu_log(", num_qgroups = %" PRIu64,
> +                     tswap64(target_inherit->num_qgroups));
> +            qemu_log(", num_ref_copies = %" PRIu64,
> +                     tswap64(target_inherit->num_ref_copies));
> +            qemu_log(", num_excl_copies = %" PRIu64,
> +                     tswap64(target_inherit->num_excl_copies));
> +
> +            qemu_log(", lim = {");
> +            qemu_log("lim.flags = %" PRIu64,
> +                     tswap64(target_inherit->lim.flags));
> +            qemu_log(", lim.max_rfer = %" PRIu64,
> +                     tswap64(target_inherit->lim.max_rfer));
> +            qemu_log(", lim.max_excl = %" PRIu64,
> +                     tswap64(target_inherit->lim.max_excl));
> +            qemu_log(", lim.rsv_rfer = %" PRIu64,
> +                     tswap64(target_inherit->lim.rsv_rfer));
> +            qemu_log(", lim.rsv_excl = %" PRIu64 "}",
> +                     tswap64(target_inherit->lim.rsv_excl));
> +
> +            qemu_log(", ...}, ");
> +        }
> +
> +        unlock_user(target_inherit, inherit_addr, 0);
> +    }
> +#endif
> +
> +#ifdef BTRFS_DEVICE_SPEC_BY_ID
> +    if (flags & BTRFS_DEVICE_SPEC_BY_ID) {
> +        qemu_log(" devid = %" PRIu64, tswap64(target_args_v2->devid));
> +        device_spec_by_id = 1;
> +    }
> +#endif
> +#ifdef BTRFS_SUBVOL_SPEC_BY_ID
> +    if (flags & BTRFS_SUBVOL_SPEC_BY_ID) {
> +        qemu_log(" subvolid = %" PRIu64, tswap64(target_args_v2->subvolid));
> +        subvol_spec_by_id = 1;
> +    }
> +#endif
> +    if (!device_spec_by_id && !subvol_spec_by_id) {
> +        qemu_log(" name = \"%s\"", target_args_v2->name);

If the name is not NUL terminated we can have an overflow here.

> +    }
> +
> +    qemu_log("}");
> +}
> +#endif
> +
>  #undef UNUSED
>  
>  #ifdef TARGET_NR_accept
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index b58098b5bb..78d7106b80 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -5668,6 +5668,83 @@ static const StructEntry struct_termios_def = {
>      .print = print_termios,
>  };
>  
> +#if defined(BTRFS_IOC_SUBVOL_CREATE_V2) || defined(BTRFS_IOC_SNAP_CREATE_V2) 
> \
> + || defined(BTRFS_SNAP_DESTROY_V2)      || defined(BTRFS_RM_DEV_V2)
> +static void target_to_host_btrfs_ioctl_vol_args_v2(void *dst, const void 
> *src)
> +{
> +    struct btrfs_ioctl_vol_args_v2 *host_args_v2 = dst;
> +    const struct target_btrfs_ioctl_vol_args_v2 *target_args_v2 = src;
> +    int device_spec_by_id = 0;
> +    int subvol_spec_by_id = 0;
> +
> +    __get_user(host_args_v2->fd, &target_args_v2->fd);
> +    __get_user(host_args_v2->flags, &target_args_v2->flags);
> +    __get_user(host_args_v2->transid, &target_args_v2->transid);
> +
> +    if (host_args_v2->flags & BTRFS_SUBVOL_QGROUP_INHERIT) {
> +        struct btrfs_qgroup_inherit *target_inherit;
> +        abi_long inherit_addr = tswapal(target_args_v2->qgroup_inherit);
> +
> +        target_inherit = lock_user(VERIFY_READ, inherit_addr,
> +                                   sizeof(*target_inherit), 1);
> +
> +        host_args_v2->qgroup_inherit = g_new(struct btrfs_qgroup_inherit, 1);

Where do you free this memory?
And the size is in fact variable and given by size in
btrfs_ioctl_vol_args_v2.

> +
> +        if (!target_inherit) {
> +            host_args_v2->qgroup_inherit = NULL;

And you overwrite the pointer here

> +        } else {
> +            __get_user(host_args_v2->qgroup_inherit->flags,
> +                       &target_inherit->flags);
> +            __get_user(host_args_v2->qgroup_inherit->num_qgroups,
> +                       &target_inherit->num_qgroups);
> +            __get_user(host_args_v2->qgroup_inherit->num_ref_copies,
> +                       &target_inherit->num_ref_copies);
> +            __get_user(host_args_v2->qgroup_inherit->num_excl_copies,
> +                       &target_inherit->num_excl_copies);
> +            __get_user(host_args_v2->qgroup_inherit->lim.flags,
> +                       &target_inherit->lim.flags);
> +            __get_user(host_args_v2->qgroup_inherit->lim.max_rfer,
> +                       &target_inherit->lim.max_rfer);
> +            __get_user(host_args_v2->qgroup_inherit->lim.max_excl,
> +                       &target_inherit->lim.max_excl);
> +            __get_user(host_args_v2->qgroup_inherit->lim.rsv_rfer,
> +                       &target_inherit->lim.rsv_rfer);
> +            __get_user(host_args_v2->qgroup_inherit->lim.rsv_excl,
> +                       &target_inherit->lim.rsv_excl);

And what about qgroups?

> +        }
> +
> +        unlock_user(target_inherit, inherit_addr, 0);
> +    }
> +
> +#ifdef BTRFS_DEVICE_SPEC_BY_ID
> +    if (host_args_v2->flags & BTRFS_DEVICE_SPEC_BY_ID) {
> +        __get_user(host_args_v2->devid, &target_args_v2->devid);
> +        device_spec_by_id = 1;
> +    }
> +#endif
> +#ifdef BTRFS_SUBVOL_SPEC_BY_ID
> +    if (host_args_v2->flags & BTRFS_SUBVOL_SPEC_BY_ID) {
> +        __get_user(host_args_v2->subvolid, &target_args_v2->subvolid);
> +        subvol_spec_by_id = 1;
> +    }
> +#endif
> +
> +    if (!device_spec_by_id & !subvol_spec_by_id) {
> +        memcpy(host_args_v2->name, target_args_v2->name,
> +               BTRFS_SUBVOL_NAME_MAX + 1);
> +    }
> +}
> +
> +static const StructEntry struct_btrfs_ioctl_vol_args_v2_def = {
> +    .convert = { NULL, target_to_host_btrfs_ioctl_vol_args_v2},
> +    .print = print_btrfs_ioctl_vol_args_v2,
> +    .align = { __alignof__(struct target_btrfs_ioctl_vol_args_v2),
> +               __alignof__(struct btrfs_ioctl_vol_args_v2) },
> +    .size = { sizeof(struct target_btrfs_ioctl_vol_args_v2),
> +              sizeof(struct btrfs_ioctl_vol_args_v2) },
> +};
> +#endif
> +
>  static bitmask_transtbl mmap_flags_tbl[] = {
>      { TARGET_MAP_SHARED, TARGET_MAP_SHARED, MAP_SHARED, MAP_SHARED },
>      { TARGET_MAP_PRIVATE, TARGET_MAP_PRIVATE, MAP_PRIVATE, MAP_PRIVATE },
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 969377d622..d2d4d1e1cb 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -967,6 +967,26 @@ struct target_rtc_pll_info {
>  #define TARGET_FS_IOC32_GETVERSION TARGET_IOR('v', 1, int)
>  #define TARGET_FS_IOC32_SETVERSION TARGET_IOW('v', 2, int)
>  
> +#define TARGET_BTRFS_SUBVOL_NAME_MAX    4039

As you use BTRFS_SUBVOL_NAME_MAX in memcpy() of
target_to_host_btrfs_ioctl_vol_args_v2, I think you don't need to define
the TARGET_ variant (the value is the same for all the targets).

> +
> +struct target_btrfs_ioctl_vol_args_v2 {
> +    uint64_t fd;

fd is signed in btrfs_ioctl_vol_args_v2

> +    uint64_t transid;
> +    uint64_t flags;
> +    union {
> +        struct {
> +            uint64_t size;
> +            abi_long qgroup_inherit;
> +        };
> +        uint64_t unused[4];
> +    };
> +    union {
> +        char name[TARGET_BTRFS_SUBVOL_NAME_MAX + 1];
> +        uint64_t devid;
> +        uint64_t subvolid;
> +    };
> +};
> +
>  /* btrfs ioctls */
>  #define TARGET_BTRFS_IOC_SNAP_CREATE            
> TARGET_IOWU(BTRFS_IOCTL_MAGIC, 1)
>  #define TARGET_BTRFS_IOC_SCAN_DEV               
> TARGET_IOWU(BTRFS_IOCTL_MAGIC, 4)

Thanks,
Laurent