| Message ID | 64e28e67cbab0a2cd97411b848911414a743d83f.1427705646.git.osandov@osandov.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 30.03.2015 12:02, 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(). > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=93021 > Reported-by: Timo Kokkonen <timo.kokkonen@offcode.fi> > Fixes: bafc9b754f75 ("vfs: More precise tests in d_invalidate") > Signed-off-by: Omar Sandoval <osandov@osandov.com> Tested-by: Timo Kokkonen <timo.kokkonen@offcode.fi> Thank you -Timo > --- > 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? > > fs/btrfs/ioctl.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 74609b9..39b0538 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -2384,6 +2384,12 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file, > goto out_dput; > } > > + spin_lock(&dentry->d_lock); > + err = dentry->d_lockref.count > 1 ? -EBUSY : 0; > + spin_unlock(&dentry->d_lock); > + if (err) > + goto out_dput; > + > mutex_lock(&inode->i_mutex); > > /* > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 74609b9..39b0538 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -2384,6 +2384,12 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file, goto out_dput; } + spin_lock(&dentry->d_lock); + err = dentry->d_lockref.count > 1 ? -EBUSY : 0; + spin_unlock(&dentry->d_lock); + if (err) + goto out_dput; + mutex_lock(&inode->i_mutex); /*
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(). Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=93021 Reported-by: Timo Kokkonen <timo.kokkonen@offcode.fi> Fixes: bafc9b754f75 ("vfs: More precise tests in d_invalidate") Signed-off-by: Omar Sandoval <osandov@osandov.com> --- 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? fs/btrfs/ioctl.c | 6 ++++++ 1 file changed, 6 insertions(+)