From patchwork Mon Mar 30 18:41:35 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Omar Sandoval X-Patchwork-Id: 6123731 Return-Path: X-Original-To: patchwork-linux-btrfs@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 67AAE9F2EC for ; Mon, 30 Mar 2015 18:42:04 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 635A52035B for ; Mon, 30 Mar 2015 18:42:03 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 4FEA220303 for ; Mon, 30 Mar 2015 18:42:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752954AbbC3Slj (ORCPT ); Mon, 30 Mar 2015 14:41:39 -0400 Received: from mail-pd0-f181.google.com ([209.85.192.181]:34767 "EHLO mail-pd0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752752AbbC3Slh (ORCPT ); Mon, 30 Mar 2015 14:41:37 -0400 Received: by pdbni2 with SMTP id ni2so183896268pdb.1 for ; Mon, 30 Mar 2015 11:41:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=2IxPQzlEsUcjaITLai+BTOtoRb8M3pWj4XdOuNVsfRM=; b=eytpAb6ijnKlbcHG0Fdc63KV6biaas0qU3gyE03YXU/VNSzAnEEBqQt1qH88A685rv e0PoWiuqLEquNoTe5wPFh8AFoj+O2V4Yq3cXRzcuFcfG8P/ZAHZ3zQMmSwFJnS30Bx8b XG69cAy+qgP3cMc7E2V4taO4UNrFiUkvzu59iKMt+4VfMqKLs3CZr37GpVDIuMnrzgo9 XMiwSios6cCpE5CdwD/x5xPSJ1B4TYjcjr1w++tPW7luggLY1aWh1Kay+WZLDLwdALlz R7O4maX/sruxCpnFPocM2qDSxLsMGLqxSm+sEXEQdgSQJ4eExQslcEfUCHkpW2NsZeoi 7ltA== X-Gm-Message-State: ALoCoQklvUO96SaUsZLiHRnKwnlOZCK5gLObsiJj0LzAufY/HVlLxulJPT1iUpI5+bw8WdqUoXUl X-Received: by 10.70.35.193 with SMTP id k1mr61394614pdj.46.1427740897047; Mon, 30 Mar 2015 11:41:37 -0700 (PDT) Received: from mew.dhcp4.washington.edu (D-108-179-185-168.dhcp4.washington.edu. [108.179.185.168]) by mx.google.com with ESMTPSA id dj9sm6147785pdb.4.2015.03.30.11.41.36 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 30 Mar 2015 11:41:36 -0700 (PDT) Date: Mon, 30 Mar 2015 11:41:35 -0700 From: Omar Sandoval To: dsterba@suse.cz, Chris Mason , Josef Bacik , "Eric W. Biederman" , Timo Kokkonen , linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org Subject: Re: [PATCH] Btrfs: prevent deletion of mounted subvolumes Message-ID: <20150330184135.GA27227@mew.dhcp4.washington.edu> References: <64e28e67cbab0a2cd97411b848911414a743d83f.1427705646.git.osandov@osandov.com> <20150330123034.GB32051@suse.cz> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20150330123034.GB32051@suse.cz> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-btrfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-btrfs@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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. > > 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). ---- ---- 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. */