| Message ID | 20210726102816.612434-16-brauner@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: support idmapped mounts | expand |
On 2021/7/26 下午6:28, Christian Brauner wrote: > From: Christian Brauner <christian.brauner@ubuntu.com> > > So far we prevented the deletion of subvolumes and snapshots using subvolume > ids possible with the BTRFS_SUBVOL_SPEC_BY_ID flag. > This restriction is necessary on idmapped mounts as this allows filesystem wide > subvolume and snapshot deletions and thus can escape the scope of what's > exposed under the mount identified by the fd passed with the ioctl. > > Deletion by subvolume id works by looking for an alias of the parent of the > subvolume or snapshot to be deleted. The parent alias can be anywhere in the > filesystem. However, as long as the alias of the parent that is found is the > same as the one identified by the file descriptor passed through the ioctl we > can allow the deletion. > > Cc: Chris Mason <clm@fb.com> > Cc: Josef Bacik <josef@toxicpanda.com> > Cc: Christoph Hellwig <hch@infradead.org> > Cc: David Sterba <dsterba@suse.com> > Cc: linux-btrfs@vger.kernel.org > Reviewed-by: Josef Bacik <josef@toxicpanda.com> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Although I'm wondering if there is any special use case for this relaxed subvolid deletion? As for most subvolume deletion it's from btrfs-progs, which has extra path lookup before calling the ioctl (even for subvolid id deletion). Thus I guess the relaxed check is only for direct ioctl call for subvolume deletion? Thanks, Qu > --- > /* v2 */ > unchanged > > /* v3 */ > unchanged > --- > fs/btrfs/ioctl.c | 27 ++++++++++++++++----------- > 1 file changed, 16 insertions(+), 11 deletions(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 488e2395034f..b9864d63ffbf 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -2944,17 +2944,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file, > if (err) > goto out; > } else { > - /* > - * Deleting by subvolume id can be used to delete > - * subvolumes/snapshots anywhere in the filesystem. > - * Ensure that users can't abuse idmapped mounts of > - * btrfs subvolumes/snapshots to perform operations in > - * the whole filesystem. > - */ > - if (mnt_userns != &init_user_ns) { > - err = -EOPNOTSUPP; > - goto out; > - } > + struct inode *old_dir; > > if (vol_args2->subvolid < BTRFS_FIRST_FREE_OBJECTID) { > err = -EINVAL; > @@ -2992,6 +2982,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file, > err = PTR_ERR(parent); > goto out_drop_write; > } > + old_dir = dir; > dir = d_inode(parent); > > /* > @@ -3002,6 +2993,20 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file, > */ > destroy_parent = true; > > + /* > + * On idmapped mounts, deletion via subvolid is > + * restricted to subvolumes that are immediate > + * ancestors of the inode referenced by the file > + * descriptor in the ioctl. Otherwise the idmapping > + * could potentially be abused to delete subvolumes > + * anywhere in the filesystem the user wouldn't be able > + * to delete without an idmapped mount. > + */ > + if (old_dir != dir && mnt_userns != &init_user_ns) { > + err = -EOPNOTSUPP; > + goto free_parent; > + } > + > subvol_name_ptr = btrfs_get_subvol_name_from_objectid( > fs_info, vol_args2->subvolid); > if (IS_ERR(subvol_name_ptr)) { >
On Mon, Jul 26, 2021 at 07:31:07PM +0800, Qu Wenruo wrote: > > > On 2021/7/26 下午6:28, Christian Brauner wrote: > > From: Christian Brauner <christian.brauner@ubuntu.com> > > > > So far we prevented the deletion of subvolumes and snapshots using subvolume > > ids possible with the BTRFS_SUBVOL_SPEC_BY_ID flag. > > This restriction is necessary on idmapped mounts as this allows filesystem wide > > subvolume and snapshot deletions and thus can escape the scope of what's > > exposed under the mount identified by the fd passed with the ioctl. > > > > Deletion by subvolume id works by looking for an alias of the parent of the > > subvolume or snapshot to be deleted. The parent alias can be anywhere in the > > filesystem. However, as long as the alias of the parent that is found is the > > same as the one identified by the file descriptor passed through the ioctl we > > can allow the deletion. > > > > Cc: Chris Mason <clm@fb.com> > > Cc: Josef Bacik <josef@toxicpanda.com> > > Cc: Christoph Hellwig <hch@infradead.org> > > Cc: David Sterba <dsterba@suse.com> > > Cc: linux-btrfs@vger.kernel.org > > Reviewed-by: Josef Bacik <josef@toxicpanda.com> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > Although I'm wondering if there is any special use case for this relaxed > subvolid deletion? > > As for most subvolume deletion it's from btrfs-progs, which has extra > path lookup before calling the ioctl (even for subvolid id deletion). > > Thus I guess the relaxed check is only for direct ioctl call for > subvolume deletion? Yeah, it's for convenience. My think was that it might be helpful when you already have the subvolume id around and can use it for subvolume deletion not bothering with paths. Christian
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 488e2395034f..b9864d63ffbf 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -2944,17 +2944,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file, if (err) goto out; } else { - /* - * Deleting by subvolume id can be used to delete - * subvolumes/snapshots anywhere in the filesystem. - * Ensure that users can't abuse idmapped mounts of - * btrfs subvolumes/snapshots to perform operations in - * the whole filesystem. - */ - if (mnt_userns != &init_user_ns) { - err = -EOPNOTSUPP; - goto out; - } + struct inode *old_dir; if (vol_args2->subvolid < BTRFS_FIRST_FREE_OBJECTID) { err = -EINVAL; @@ -2992,6 +2982,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file, err = PTR_ERR(parent); goto out_drop_write; } + old_dir = dir; dir = d_inode(parent); /* @@ -3002,6 +2993,20 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file, */ destroy_parent = true; + /* + * On idmapped mounts, deletion via subvolid is + * restricted to subvolumes that are immediate + * ancestors of the inode referenced by the file + * descriptor in the ioctl. Otherwise the idmapping + * could potentially be abused to delete subvolumes + * anywhere in the filesystem the user wouldn't be able + * to delete without an idmapped mount. + */ + if (old_dir != dir && mnt_userns != &init_user_ns) { + err = -EOPNOTSUPP; + goto free_parent; + } + subvol_name_ptr = btrfs_get_subvol_name_from_objectid( fs_info, vol_args2->subvolid); if (IS_ERR(subvol_name_ptr)) {