From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753596AbbDAD7B (ORCPT ); Tue, 31 Mar 2015 23:59:01 -0400 Received: from out02.mta.xmission.com ([166.70.13.232]:60898 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752893AbbDAD6z (ORCPT ); Tue, 31 Mar 2015 23:58:55 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: Omar Sandoval Cc: dsterba@suse.cz, Chris Mason , Josef Bacik , Timo Kokkonen , linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org References: <64e28e67cbab0a2cd97411b848911414a743d83f.1427705646.git.osandov@osandov.com> <20150330123034.GB32051@suse.cz> <20150330184135.GA27227@mew.dhcp4.washington.edu> Date: Tue, 31 Mar 2015 22:54:55 -0500 In-Reply-To: <20150330184135.GA27227@mew.dhcp4.washington.edu> (Omar Sandoval's message of "Mon, 30 Mar 2015 11:41:35 -0700") Message-ID: <87k2xwv6y8.fsf@x220.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-AID: U2FsdGVkX1/5TzJ8aBapuU2eTeQmNHn3kKbC1csM6js= X-SA-Exim-Connect-IP: 70.59.163.10 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.0 TVD_RCVD_IP Message was received from an IP address * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.5000] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa07 1397; Body=1 Fuz1=1 Fuz2=1] X-Spam-DCC: XMission; sa07 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: ;Omar Sandoval X-Spam-Relay-Country: X-Spam-Timing: total 612 ms - load_scoreonly_sql: 0.05 (0.0%), signal_user_changed: 6 (0.9%), b_tie_ro: 3.9 (0.6%), parse: 1.86 (0.3%), extract_message_metadata: 24 (3.9%), get_uri_detail_list: 4.6 (0.7%), tests_pri_-1000: 7 (1.2%), tests_pri_-950: 1.49 (0.2%), tests_pri_-900: 1.34 (0.2%), tests_pri_-400: 38 (6.3%), check_bayes: 37 (6.0%), b_tokenize: 12 (1.9%), b_tok_get_all: 12 (2.0%), b_comp_prob: 6 (0.9%), b_tok_touch_all: 4.2 (0.7%), b_finish: 1.07 (0.2%), tests_pri_0: 520 (84.9%), tests_pri_500: 7 (1.2%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH] Btrfs: prevent deletion of mounted subvolumes X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Wed, 24 Sep 2014 11:00:52 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Omar Sandoval writes: > On Mon, Mar 30, 2015 at 02:30:34PM +0200, David Sterba wrote: >> On Mon, Mar 30, 2015 at 02:02:17AM -0700, Omar Sandoval wrote: >> > Before commit bafc9b754f75 ("vfs: More precise tests in d_invalidate"), >> > d_invalidate() could return -EBUSY when a dentry for a directory had >> > more than one reference to it. This is what prevented a mounted >> > subvolume from being deleted, as struct vfsmount holds a reference to >> > the subvolume dentry. However, that commit removed that case, and later >> > commits in that patch series removed the return code from d_invalidate() >> > completely, so we don't get that check for free anymore. So, reintroduce >> > it in btrfs_ioctl_snap_destroy(). >> >> > This applies to 4.0-rc6. To be honest, I'm not sure that this is the most >> > correct fix for this bug, but it's equivalent to the pre-3.18 behavior and it's >> > the best that I could come up with. Thoughts? >> >> > + spin_lock(&dentry->d_lock); >> > + err = dentry->d_lockref.count > 1 ? -EBUSY : 0; >> > + spin_unlock(&dentry->d_lock); >> >> The fix restores the original behaviour, but I don't think opencoding and >> using internals is fine. Either there should be a vfs api for that or >> there's an existing one that can be used instead. I have a problem with restoring the original behavior as is. In some sense it re-introduces the security issue that the d_invalidate changes were built to fix. Any user in the system can create a user namespace, create a mount namespace and keep any subvolume pinned forever. Which at the very least could make a very nice DOS attack. I am not familiar enough with how people use subvolumes and So let me ask. How can userspace not know that a subvolume that they want to delete is already mounted? I can see having something like is_local_mount_root and denying the subvolume destruction if the mount that is pinning it is in your local mount namespace. >> The bug here seems defined up to the point that we're trying to delete a >> subvolume that's a mountpoint. My next guess is that a check >> >> if (d_mountpoint(&dentry)) { ... } >> >> could work. > > That was my first instinct as well, but d_mountpoint() is true for > dentries that have a filesystem mounted on them (e.g., after mount > /dev/sda1 /mnt, the dentry for "/mnt"), not the dentry that is mounted. > > I poked around the mount code for awhile and couldn't come up with > anything using the existing interface. Mounting subvolumes bubbles down > to mount_subtree(), which doesn't really leave any traces of which > subvolume is mounted except for the dentry in struct vfsmount. > > (As far as I can tell, under the covers subvolume deletion is more or > less equivalent to an rm -rf, and we obviously don't do anything to stop > users from doing that on the root of their mounted filesystem, but it > appears that users expect the original behavior.) > > Here's an idea: mark mount root dentries as such in the VFS and check it > in the Btrfs code. Adding fsdevel ML for comments > (https://lkml.org/lkml/2015/3/30/125 is the original message). Marking root dentries is needed to fix the bug that you can escape the limitations of loopback mounts with a carefully placed rename. I have a patch cooking that marks mountpoints and tracks all of the mounts on a dentry. So except for the possibility of stepping on each others toes I have no objections. Eric > ---- > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 74609b9..8a0933d 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -2384,6 +2384,11 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file, > goto out_dput; > } > > + if (d_is_mount_root(dentry)) { > + err = -EBUSY; > + goto out_dput; > + } > + > mutex_lock(&inode->i_mutex); > > /* > diff --git a/fs/namespace.c b/fs/namespace.c > index 82ef140..a28ca15 100644 > --- a/fs/namespace.c > +++ b/fs/namespace.c > @@ -920,6 +920,10 @@ vfs_kern_mount(struct file_system_type *type, int flags, const char *name, void > return ERR_CAST(root); > } > > + spin_lock(&root->d_lock); > + root->d_flags |= DCACHE_MOUNT_ROOT; > + spin_unlock(&root->d_lock); > + > mnt->mnt.mnt_root = root; > mnt->mnt.mnt_sb = root->d_sb; > mnt->mnt_mountpoint = mnt->mnt.mnt_root; > @@ -1017,6 +1021,8 @@ static struct mount *clone_mnt(struct mount *old, struct dentry *root, > > static void cleanup_mnt(struct mount *mnt) > { > + struct dentry *root = mnt->mnt.mnt_root; > + > /* > * This probably indicates that somebody messed > * up a mnt_want/drop_write() pair. If this > @@ -1031,7 +1037,10 @@ static void cleanup_mnt(struct mount *mnt) > if (unlikely(mnt->mnt_pins.first)) > mnt_pin_kill(mnt); > fsnotify_vfsmount_delete(&mnt->mnt); > - dput(mnt->mnt.mnt_root); > + spin_lock(&root->d_lock); > + root->d_flags &= ~DCACHE_MOUNT_ROOT; > + spin_unlock(&root->d_lock); > + dput(root); > deactivate_super(mnt->mnt.mnt_sb); > mnt_free_id(mnt); > call_rcu(&mnt->mnt_rcu, delayed_free_vfsmnt); > diff --git a/include/linux/dcache.h b/include/linux/dcache.h > index d835879..d974ab8 100644 > --- a/include/linux/dcache.h > +++ b/include/linux/dcache.h > @@ -225,6 +225,7 @@ struct dentry_operations { > > #define DCACHE_MAY_FREE 0x00800000 > #define DCACHE_FALLTHRU 0x01000000 /* Fall through to lower layer */ > +#define DCACHE_MOUNT_ROOT 0x02000000 /* is the root of a mount */ > > extern seqlock_t rename_lock; > > @@ -401,6 +402,16 @@ static inline bool d_mountpoint(const struct dentry *dentry) > return dentry->d_flags & DCACHE_MOUNTED; > } > > +static inline bool d_is_mount_root(const struct dentry *dentry) > +{ > + bool ret; > + > + spin_lock(&dentry->d_lock); > + ret = dentry->d_flags & DCACHE_MOUNT_ROOT; > + spin_unlock(&dentry->d_lock); > + return ret; > +} > + > /* > * Directory cache entry type accessor functions. > */ > ----