lkml.org 
[lkml]   [2020]   [Jan]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCHv2] btrfs: Introduce new BTRFS_IOC_SNAP_DESTROY_V2 ioctl
From
Date
On Tue, 2020-01-28 at 18:26 +0100, David Sterba wrote:
> wrote:
> > From: Marcos Paulo de Souza <mpdesouza@suse.com>

...

> > struct dentry *parent = file->f_path.dentry;
> > struct btrfs_fs_info *fs_info = btrfs_sb(parent->d_sb);
> > @@ -2845,34 +2847,87 @@ 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;
> > + struct btrfs_ioctl_vol_args *vol_args = NULL;
> > + struct btrfs_ioctl_vol_args_v2 *vol_args2 = NULL;
> > + char *name, *name_ptr = NULL;
>
> The naming is confusing, name_ptr refers to the resolved subvolume
> name,
> so I suggest to rename it to subvol_name.

Sure, much better.

> > int namelen;
> > int err = 0;
> >
> > - 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 (vol_args2->subvolid == 0) {
>
> This should be compared >= BTRFS_FIRST_FREE_OBJECTID
> as there are no valid subvolumes with lower id. The exception is the
> toplevel subvolume with id 5 that must not be deletable.

Agreed.

>
> > + err = -EINVAL;
> > + goto out;
> > + }
> >
> > - 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;
> > + if (!(vol_args2->flags & BTRFS_SUBVOL_BY_ID)) {
> > + err = -EINVAL;
> > + goto out;
>
> The flag validation needs to be factored out of the if. First
> validate,
> then do the rest. For backward compatibility, the v1 ioctl must take
> no
> flags, so if theres BTRFS_SUBVOL_BY_ID for v1, it needs to fail. For
> v2
> the flag is optional.

Only vol_args_v2 has the flags field, so for current
BTRFS_IOC_SNAP_DESTORY there won't be any flags. If we drop the check
for BTRFS_SUBVOL_BY_ID in BTRFS_IOC_SNAP_DESTORY_V2, so won't check for
this flag at all, making it meaningless.

What do you think? Should we drop this flag at all and just rely in the
ioctl number + subvolid being informed?

>
> > + }
> > +
> > + 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;
> > + }
> > +
> > + /*
>
> There's a trailing space on the line, 'git am' does not allow me to
> apply the patch without removing it manually. Same for the comment
> below.

Done.

>
> > + * change the default parent since the subvolume being
> deleted
>
> Also please uppercase first letter in comments unless it's an
> identifier. I fix such things but for patches that are going to have
> another iteration it's better to point it out.

Sure.

>
> > + * can be outside of the current mount point
> > + */
> > + parent = btrfs_get_parent(dentry);
> > +
> > + /*
> > + * the only use of dentry was to get the parent, so we
> can
> > + * release it now. Later on the dentry will be queried
> again to
> > + * make sure the dentry will reside in the dentry cache
>
> Can you please rephrase that? I'm not sure I understand.

What do you think about:
/*
* 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.
*/


> > + if (strchr(vol_args->name, '/') ||
> + strncmp(vol_args->name, "..", namelen) == 0) {
> > + err = -EINVAL;
> > + goto out;
> > + }
>
> This sanity check can be unconditional, ie. also done for the v2 even
> in
> the spec-by-id case.

Makes sense.

>
> > + name = vol_args->name;
> > + }
> > +
> > + if (!S_ISDIR(dir->i_mode)) {
> > + err = -ENOTDIR;
> > + goto free_subvol_name;
> > }
> >
> > err = mnt_want_write_file(file);
>
> So this is related to separating the validation. Calling
> mnt_want_write_file must be between flag validation and using the
> dentries and resolving path etc.
>
> The initial part is ordered like: argument checks, subsystem checks,
> the
> implementation.

Done.

Thanks for your review David. Once I have the flag question clarified I
will sent the v3.


Thanks,
Marcos

\
 
 \ /
  Last update: 2020-01-29 16:05    [W:0.292 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site