[SRU,Disco] UBUNTU: SAUCE: shiftfs: fix buggy unlink logic
diff mbox series

Message ID 20190829184507.21173-1-christian.brauner@ubuntu.com
State New
Headers show
Series
  • [SRU,Disco] UBUNTU: SAUCE: shiftfs: fix buggy unlink logic
Related show

Commit Message

Christian Brauner Aug. 29, 2019, 6:45 p.m. UTC
BugLink: https://bugs.launchpad.net/bugs/1841977

The way we messed with setting i_nlink was brittle and wrong. We used to
set the i_nlink of the shiftfs dentry to be deleted to the i_nlink count
of the underlay dentry of the directory it resided in which makes no
sense whatsoever. We also missed drop_nlink() which is crucial since
i_nlink affects whether a dentry is cleaned up on dput().
With this I cannot reproduce the bug anymore where shiftfs misleads zfs
into believing that a deleted file can not be removed from disk because
it is still referenced.

Fixes: commit 87011da41961 ("shiftfs: rework and extend")
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 fs/shiftfs.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Comments

Seth Forshee Aug. 30, 2019, 5:04 p.m. UTC | #1
On Thu, Aug 29, 2019 at 08:45:07PM +0200, Christian Brauner wrote:
> BugLink: https://bugs.launchpad.net/bugs/1841977
> 
> The way we messed with setting i_nlink was brittle and wrong. We used to
> set the i_nlink of the shiftfs dentry to be deleted to the i_nlink count
> of the underlay dentry of the directory it resided in which makes no
> sense whatsoever. We also missed drop_nlink() which is crucial since
> i_nlink affects whether a dentry is cleaned up on dput().
> With this I cannot reproduce the bug anymore where shiftfs misleads zfs
> into believing that a deleted file can not be removed from disk because
> it is still referenced.

Obviously the intent was not to copy i_nlink from the corresponding
inode in the lower filesystem, not the directory inode. Seems likely
that some confusing/inconsistent variable naming contributed to the
mistake.

As I mentioned on irc, I'm conflicted on whether we should just
decrement the reference count like this or copy from the lower fs inode.
It should only matter if the lower fs is being modified underneath
shiftfs, and shiftfs cannot promise correct behavior if that happens, so
this should be ok.

Acked-by: Seth Forshee <seth.forshee@canonical.com>

Please remember to submit these for the development release as well.
Applied to eoan/master-next and unstable/master, thanks!

Patch
diff mbox series

diff --git a/fs/shiftfs.c b/fs/shiftfs.c
index 57e04a02d74c..084aa7a25566 100644
--- a/fs/shiftfs.c
+++ b/fs/shiftfs.c
@@ -585,6 +585,7 @@  static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir)
 {
 	struct dentry *lowerd = dentry->d_fsdata;
 	struct inode *loweri = dir->i_private;
+	struct inode *inode = d_inode(dentry);
 	int err;
 	const struct cred *oldcred;
 
@@ -594,15 +595,19 @@  static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir)
 		err = vfs_rmdir(loweri, lowerd);
 	else
 		err = vfs_unlink(loweri, lowerd, NULL);
-	inode_unlock(loweri);
 	revert_creds(oldcred);
 
-	shiftfs_copyattr(loweri, dir);
-	set_nlink(d_inode(dentry), loweri->i_nlink);
-	if (!err)
+	if (!err) {
 		d_drop(dentry);
 
-	set_nlink(dir, loweri->i_nlink);
+		if (rmdir)
+			clear_nlink(inode);
+		else
+			drop_nlink(inode);
+	}
+	inode_unlock(loweri);
+
+	shiftfs_copyattr(loweri, dir);
 
 	return err;
 }