On Fri, Feb 07, 2020 at 10:05:46AM -0300, Marcos Paulo de Souza wrote:
> From: Marcos Paulo de Souza <mpdesouza@xxxxxxxx>
>
> This ioctl will be responsible for deleting a subvolume using it's id.
> This can be used when a system has a file system mounted from a
> subvolume, rather than the root file system, like below:
>
> /
> |- @subvol1
> |- @subvol2
> \- @subvol_default
>
> If only @subvol_default is mounted, we have no path to reach
> @subvol1 and @subvol2, thus no way to delete them. Current subvolume
> delete ioctl takes a file handler point as argument, and if
> @subvol_default is mounted, we can't reach @subvol1 and @subvol2 from
> the same mount point.
>
> This patch introduces a new flag to allow BTRFS_IOC_SNAP_DESTROY_V2
> to delete subvolume using subvolid.
>
> Now, we can use this new ioctl specifying the subvolume id and refer to
> the same mount point. It doesn't matter which subvolume was mounted,
> since we can reach to the desired one using the subvolume id, and then
> delete it.
>
> Also in this patch:
> * export get_subvol_name_from_objectid, adding btrfs suffix
> * add BTRFS_SUBVOL_SPEC_BY_ID flag
> * add subvolid as a union member in struct btrfs_ioctl_vol_args_v2.
>
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@xxxxxxxx>
> ---
>
> Changes from v2:
> * Commit message improved, explaining how to use the new ioctl (David)
> * Moved subvolid member to the union which already contained devid and name
> (David)
> * Changed name_ptr to subvol_name_ptr, since it'll point to the "full"
> subvolume name, but we need the basename of this char, which was also renamed
> to subvol_name (David).
> * Change the check for a valid subvolid to be >= BTRFS_FIRST_FREE_OBJECTID
> (David)
> * Now BTRFS_IOC_SNAP_DESTROY_V2 can handle both cases where the user uses the
> subvolid and just the subvolume name (David)
> * Changed BTRFS_SUBVOL_DELETE_BY_ID to BTRFS_SUBVOL_SPEC_BY_ID, since this flag
> can be used for other actions rather than deleting a subvolume (David, Christoph)
> * Rewritten comment about the getting/releasing the dentry before doing the
> lookup, explaining why this dentry can be released in order to get a new one
> from lookup (David)
> * Moved mnt_want_write_file call sites right after the flag validation (David)
>
> Changes from v1:
> * make btrfs_ioctl_snap_destroy handle both SNAP_DESTROY and SNAP_DESTROY_V2
> (suggested by Josef)
> * Change BTRFS_SUBVOL_DELETE_BY_ID to BTRFS_SUBVOL_BY_ID (David)
> * Send patches for btrfs-progs and xfstests along this change
>
> fs/btrfs/ctree.h | 2 +
> fs/btrfs/export.c | 4 +-
> fs/btrfs/export.h | 5 ++
> fs/btrfs/ioctl.c | 128 +++++++++++++++++++++++++++++++------
> fs/btrfs/super.c | 4 +-
> include/uapi/linux/btrfs.h | 8 ++-
> 6 files changed, 127 insertions(+), 24 deletions(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 54efb21c2727..2d56517c4bca 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -2963,6 +2963,8 @@ int btrfs_defrag_leaves(struct btrfs_trans_handle *trans,
> int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
> unsigned long new_flags);
> int btrfs_sync_fs(struct super_block *sb, int wait);
> +char *btrfs_get_subvol_name_from_objectid(struct btrfs_fs_info *fs_info,
> + u64 subvol_objectid);
>
> static inline __printf(2, 3) __cold
> void btrfs_no_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...)
> diff --git a/fs/btrfs/export.c b/fs/btrfs/export.c
> index 72e312cae69d..027411cdbae7 100644
> --- a/fs/btrfs/export.c
> +++ b/fs/btrfs/export.c
> @@ -57,7 +57,7 @@ static int btrfs_encode_fh(struct inode *inode, u32 *fh, int *max_len,
> return type;
> }
>
> -static struct dentry *btrfs_get_dentry(struct super_block *sb, u64 objectid,
> +struct dentry *btrfs_get_dentry(struct super_block *sb, u64 objectid,
> u64 root_objectid, u32 generation,
> int check_generation)
> {
> @@ -152,7 +152,7 @@ static struct dentry *btrfs_fh_to_dentry(struct super_block *sb, struct fid *fh,
> return btrfs_get_dentry(sb, objectid, root_objectid, generation, 1);
> }
>
> -static struct dentry *btrfs_get_parent(struct dentry *child)
> +struct dentry *btrfs_get_parent(struct dentry *child)
> {
> struct inode *dir = d_inode(child);
> struct btrfs_fs_info *fs_info = btrfs_sb(dir->i_sb);
> diff --git a/fs/btrfs/export.h b/fs/btrfs/export.h
> index 57488ecd7d4e..f981e8103d64 100644
> --- a/fs/btrfs/export.h
> +++ b/fs/btrfs/export.h
> @@ -18,4 +18,9 @@ struct btrfs_fid {
> u64 parent_root_objectid;
> } __attribute__ ((packed));
>
> +struct dentry *btrfs_get_dentry(struct super_block *sb, u64 objectid,
> + u64 root_objectid, u32 generation,
> + int check_generation);
> +struct dentry *btrfs_get_parent(struct dentry *child);
> +
> #endif
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 12ae31e1813e..be5350582955 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -28,6 +28,7 @@
> #include <linux/iversion.h>
> #include "ctree.h"
> #include "disk-io.h"
> +#include "export.h"
> #include "transaction.h"
> #include "btrfs_inode.h"
> #include "print-tree.h"
> @@ -2836,7 +2837,8 @@ static int btrfs_ioctl_get_subvol_rootref(struct file *file, void __user *argp)
> }
>
> static noinline int btrfs_ioctl_snap_destroy(struct file *file,
> - void __user *arg)
> + void __user *arg,
> + bool destroy_v2)
> {
> struct dentry *parent = file->f_path.dentry;
> struct btrfs_fs_info *fs_info = btrfs_sb(parent->d_sb);
> @@ -2845,34 +2847,114 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
> struct inode *inode;
> struct btrfs_root *root = BTRFS_I(dir)->root;
> struct btrfs_root *dest = NULL;
> - struct btrfs_ioctl_vol_args *vol_args;
> - int namelen;
> + struct btrfs_ioctl_vol_args *vol_args = NULL;
> + struct btrfs_ioctl_vol_args_v2 *vol_args2 = NULL;
> + char *subvol_name, *subvol_name_ptr = NULL;
> + int subvol_namelen;
> int err = 0;
> + bool destroy_parent = false;
>
> - if (!S_ISDIR(dir->i_mode))
> - return -ENOTDIR;
> + if (destroy_v2) {
> + vol_args2 = memdup_user(arg, sizeof(*vol_args2));
> + if (IS_ERR(vol_args2))
> + return PTR_ERR(vol_args2);
>
> - vol_args = memdup_user(arg, sizeof(*vol_args));
> - if (IS_ERR(vol_args))
> - return PTR_ERR(vol_args);
> + /*
> + * If SPEC_BY_ID is not set, we are looking for the subvolume by
> + * name, same as v1 currently does.
> + */
> + if (!(vol_args2->flags & BTRFS_SUBVOL_SPEC_BY_ID)) {
> + vol_args2->name[BTRFS_PATH_NAME_MAX] = '\0';
> + subvol_name = vol_args2->name;
>
> - vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
> - namelen = strlen(vol_args->name);
> - if (strchr(vol_args->name, '/') ||
> - strncmp(vol_args->name, "..", namelen) == 0) {
> - err = -EINVAL;
> - goto out;
> + err = mnt_want_write_file(file);
> + if (err)
> + goto out;
> + } else {
> + if (vol_args2->subvolid < BTRFS_FIRST_FREE_OBJECTID) {
> + err = -EINVAL;
> + goto out;
> + }
> +
> + err = mnt_want_write_file(file);
> + if (err)
> + goto out;
> +
> + dentry = btrfs_get_dentry(fs_info->sb,
> + BTRFS_FIRST_FREE_OBJECTID,
> + vol_args2->subvolid, 0, 0);
> + if (IS_ERR(dentry)) {
> + err = PTR_ERR(dentry);
> + goto out_drop_write;
> + }
> +
> + /*
> + * Change the default parent since the subvolume being
> + * deleted can be outside of the current mount point.
> + */
> + parent = btrfs_get_parent(dentry);
> +
> + /*
> + * At this point dentry->d_name can point to '/' if the
> + * subvolume we want to destroy is outsite of the
> + * current mount point, so we need to released the
> + * current dentry and execute the lookup to return a new
> + * one with ->d_name pointing to the
> + * <mount point>/subvol_name.
> + */
> + dput(dentry);
> + if (IS_ERR(parent)) {
> + err = PTR_ERR(parent);
> + goto out_drop_write;
> + }
> + dir = d_inode(parent);
> +
> + /* If v2 was used with SPEC_BY_ID, a new parent was
> + * allocated since the subvolume can be outside of the
> + * current moutn point. Later on we need to release this
> + * new parent dentry.
> + */
> + destroy_parent = true;
> +
> + subvol_name_ptr = btrfs_get_subvol_name_from_objectid(fs_info,
> + vol_args2->subvolid);
> + if (IS_ERR(subvol_name_ptr)) {
> + err = PTR_ERR(subvol_name_ptr);
> + goto free_parent;
> + }
> + /* subvol_name_ptr is already NULL termined */
> + subvol_name = (char *)kbasename(subvol_name_ptr);
> + }
> + } else {
> + vol_args = memdup_user(arg, sizeof(*vol_args));
> + if (IS_ERR(vol_args))
> + return PTR_ERR(vol_args);
> +
> + vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
> + subvol_name = vol_args->name;
> +
> + err = mnt_want_write_file(file);
> + if (err)
> + goto out;
> }
>
> - err = mnt_want_write_file(file);
> - if (err)
> - goto out;
> + subvol_namelen = strlen(subvol_name);
>
> + if (strchr(subvol_name, '/') ||
> + strncmp(subvol_name, "..", subvol_namelen) == 0) {
> + err = -EINVAL;
> + goto free_subvol_name;
> + }
> +
> + if (!S_ISDIR(dir->i_mode)) {
> + err = -ENOTDIR;
> + goto free_subvol_name;
> + }
>
> err = down_write_killable_nested(&dir->i_rwsem, I_MUTEX_PARENT);
> if (err == -EINTR)
> goto out_drop_write;
> - dentry = lookup_one_len(vol_args->name, parent, namelen);
> + dentry = lookup_one_len(subvol_name, parent, subvol_namelen);
> if (IS_ERR(dentry)) {
> err = PTR_ERR(dentry);
> goto out_unlock_dir;
> @@ -2941,9 +3023,15 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
> dput(dentry);
> out_unlock_dir:
> inode_unlock(dir);
> +free_subvol_name:
> + kfree(subvol_name_ptr);
> +free_parent:
> + if (destroy_parent)
> + dput(parent);
> out_drop_write:
> mnt_drop_write_file(file);
> out:
> + kfree(vol_args2);
> kfree(vol_args);
> return err;
> }
> @@ -5464,7 +5552,9 @@ long btrfs_ioctl(struct file *file, unsigned int
> case BTRFS_IOC_SUBVOL_CREATE_V2:
> return btrfs_ioctl_snap_create_v2(file, argp, 1);
> case BTRFS_IOC_SNAP_DESTROY:
> - return btrfs_ioctl_snap_destroy(file, argp);
> + return btrfs_ioctl_snap_destroy(file, argp, false);
> + case BTRFS_IOC_SNAP_DESTROY_V2:
> + return btrfs_ioctl_snap_destroy(file, argp, true);
> case BTRFS_IOC_SUBVOL_GETFLAGS:
> return btrfs_ioctl_subvol_getflags(file, argp);
> case BTRFS_IOC_SUBVOL_SETFLAGS:
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index f452a94abdc3..649531e92a1d 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1005,7 +1005,7 @@ static int btrfs_parse_subvol_options(const char *options, char **subvol_name,
> return error;
> }
>
> -static char *get_subvol_name_from_objectid(struct btrfs_fs_info *fs_info,
> +char *btrfs_get_subvol_name_from_objectid(struct btrfs_fs_info *fs_info,
> u64 subvol_objectid)
> {
> struct btrfs_root *root = fs_info->tree_root;
> @@ -1417,7 +1417,7 @@ static struct dentry *mount_subvol(const char *subvol_name, u64 subvol_objectid,
> goto out;
> }
> }
> - subvol_name = get_subvol_name_from_objectid(btrfs_sb(mnt->mnt_sb),
> + subvol_name = btrfs_get_subvol_name_from_objectid(btrfs_sb(mnt->mnt_sb),
> subvol_objectid);
> if (IS_ERR(subvol_name)) {
> root = ERR_CAST(subvol_name);
> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
> index 7a8bc8b920f5..280f6ded2104 100644
> --- a/include/uapi/linux/btrfs.h
> +++ b/include/uapi/linux/btrfs.h
> @@ -42,11 +42,14 @@ struct btrfs_ioctl_vol_args {
>
> #define BTRFS_DEVICE_SPEC_BY_ID (1ULL << 3)
>
> +#define BTRFS_SUBVOL_SPEC_BY_ID (1ULL << 4)
> +
> #define BTRFS_VOL_ARG_V2_FLAGS_SUPPORTED \
> (BTRFS_SUBVOL_CREATE_ASYNC | \
> BTRFS_SUBVOL_RDONLY | \
> BTRFS_SUBVOL_QGROUP_INHERIT | \
> - BTRFS_DEVICE_SPEC_BY_ID)
> + BTRFS_DEVICE_SPEC_BY_ID | \
> + BTRFS_SUBVOL_SPEC_BY_ID)
>
> #define BTRFS_FSID_SIZE 16
> #define BTRFS_UUID_SIZE 16
> @@ -120,6 +123,7 @@ struct btrfs_ioctl_vol_args_v2 {
> };
> union {
> char name[BTRFS_SUBVOL_NAME_MAX + 1];
> + __u64 subvolid;
> __u64 devid;
> };
> };
> @@ -949,5 +953,7 @@ enum btrfs_err_code {
> struct btrfs_ioctl_get_subvol_rootref_args)
> #define BTRFS_IOC_INO_LOOKUP_USER _IOWR(BTRFS_IOCTL_MAGIC, 62, \
> struct btrfs_ioctl_ino_lookup_user_args)
> +#define BTRFS_IOC_SNAP_DESTROY_V2 _IOW(BTRFS_IOCTL_MAGIC, 63, \
> + struct btrfs_ioctl_vol_args_v2)
>
> #endif /* _UAPI_LINUX_BTRFS_H */
> --
> 2.24.0
>
Hi Marcos,
We received a build report from the 0day bot when building with clang
that appears legitimate if I am reading everything correctly.
../fs/btrfs/ioctl.c:2867:4: warning: array index 4087 is past the end of the array (which contains 4040 elements) [-Warray-bounds]
vol_args2->name[BTRFS_PATH_NAME_MAX] = '\0';
^ ~~~~~~~~~~~~~~~~~~~
../include/uapi/linux/btrfs.h:125:3: note: array 'name' declared here
char name[BTRFS_SUBVOL_NAME_MAX + 1];
^
1 warning generated.
The full report can be viewed here:
https://groups.google.com/d/msg/clang-built-linux/YFcXVkPdkTY/EhB6grZ2BQAJ
Mind taking a look at it?
Cheers,
Nathan
