linux-kernel - Re: [PATCH] Btrfs: prevent deletion of mounted subvolumes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 1 Apr 2015 20:49:54 -0700
From:	Omar Sandoval <osandov@...ndov.com>
To:	David Sterba <dsterba@...e.cz>
Cc:	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Chris Mason <clm@...com>, Josef Bacik <jbacik@...com>,
	Timo Kokkonen <timo.kokkonen@...code.fi>,
	linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] Btrfs: prevent deletion of mounted subvolumes

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:

----
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))
----

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?

-- 
Omar
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/