Btrfs: prevent deletion of mounted subvolumes

Commit Message

Omar Sandoval April 2, 2015, 3:49 a.m. UTC

On Wed, Apr 01, 2015 at 01:22:42PM +0200, David Sterba wrote:
> On Wed, Apr 01, 2015 at 12:03:28AM -0700, Omar Sandoval wrote:
> > --- a/fs/btrfs/super.c
> > +++ b/fs/btrfs/super.c
> > @@ -1024,6 +1024,10 @@ static int btrfs_show_options(struct seq_file *seq, struct dentry *dentry)
> >  	struct btrfs_root *root = info->tree_root;
> >  	char *compress_type;
> >  
> > +	if (dentry != dentry->d_sb->s_root) {
> > +		seq_puts(seq, ",subvol=");
> > +		seq_dentry(seq, dentry, " \t\n\\");
> 
> Unfortunatelly this does not work if the default subvolume is not the
> toplevel one and the implicit mount (ie. without subvol=) is used. Then
> this leads to subvol=/ although it should be subvol=/the/default .
> 
> There was a patch to build the path in the show_options callback, but it
> looked too heavy (taking locks, doing lookups). This is unrelated to the
> problem reported by Timo, though the fix might also fix this one.

Hm, yeah, that's unfortunate, thanks for pointing that out. It looks
like we can get the subvolume ID reliably:

----
----

With that, userspace has enough information to determine whether a
subvolume is mounted. That would be racy with concurrent mounts,
though...

Just to throw another idea out there, what about doing something like my
VFS patch, but then making it optional whether the kernel should error
out on a mounted subvolume, e.g., with a flag to the ioctl? btrfs-progs
could default to the original EBUSY behavior for users who depend on it,
but we could add a "force" flag to `btrfs subvolume delete` in order to
avert the DoS situation Eric wants to avoid. Thoughts on that?

Comments

David Sterba April 2, 2015, 3:02 p.m. UTC | #1

On Wed, Apr 01, 2015 at 08:49:54PM -0700, Omar Sandoval wrote:
> Hm, yeah, that's unfortunate, thanks for pointing that out. It looks
> like we can get the subvolume ID reliably:
> 
> ----
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 05fef19..a74ddb3 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1024,6 +1024,8 @@ static int btrfs_show_options(struct seq_file *seq, struct dentry *dentry)
>  	struct btrfs_root *root = info->tree_root;
>  	char *compress_type;
>  
> +	seq_printf(seq, ",subvolid=%llu",
> +		  BTRFS_I(d_inode(dentry))->root->root_key.objectid);

yes, subvolid is reliable, but not very friendly from users' POV. I'd
like to see subvol=/path there. Possibly we can have both.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Omar Sandoval April 3, 2015, 9:08 p.m. UTC | #2

On Thu, Apr 02, 2015 at 05:02:25PM +0200, David Sterba wrote:
> On Wed, Apr 01, 2015 at 08:49:54PM -0700, Omar Sandoval wrote:
> > Hm, yeah, that's unfortunate, thanks for pointing that out. It looks
> > like we can get the subvolume ID reliably:
> > 
> > ----
> > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> > index 05fef19..a74ddb3 100644
> > --- a/fs/btrfs/super.c
> > +++ b/fs/btrfs/super.c
> > @@ -1024,6 +1024,8 @@ static int btrfs_show_options(struct seq_file *seq, struct dentry *dentry)
> >  	struct btrfs_root *root = info->tree_root;
> >  	char *compress_type;
> >  
> > +	seq_printf(seq, ",subvolid=%llu",
> > +		  BTRFS_I(d_inode(dentry))->root->root_key.objectid);
> 
> yes, subvolid is reliable, but not very friendly from users' POV. I'd
> like to see subvol=/path there. Possibly we can have both.

So are you saying that we should try to fix the bug in userspace by
first getting subvol/subvolid in /proc/mounts, or do you want to keep
that a separate issue? Just asking because you didn't comment on the
kernel-side fix. Here's a concrete implementation of what I'm talking
about: https://github.com/osandov/linux/tree/btrfs-delete-mounted-subvol.

Patch

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 05fef19..a74ddb3 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1024,6 +1024,8 @@  static int btrfs_show_options(struct seq_file *seq, struct dentry *dentry)
 	struct btrfs_root *root = info->tree_root;
 	char *compress_type;
 
+	seq_printf(seq, ",subvolid=%llu",
+		  BTRFS_I(d_inode(dentry))->root->root_key.objectid);
 	if (btrfs_test_opt(root, DEGRADED))
 		seq_puts(seq, ",degraded");
 	if (btrfs_test_opt(root, NODATASUM))