| Message ID | 20190829184507.21173-1-christian.brauner@ubuntu.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,Disco] UBUNTU: SAUCE: shiftfs: fix buggy unlink logic | expand |
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!
On 29.08.19 20:45, 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. > > Fixes: commit 87011da41961 ("shiftfs: rework and extend") > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- > fs/shiftfs.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > 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; > } >
On 8/29/19 11:45 AM, 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. > > Fixes: commit 87011da41961 ("shiftfs: rework and extend") > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> Acked-by: Connor Kuehl <connor.kuehl@canonical.com> > --- > fs/shiftfs.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > 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; > } >
On 29.08.19 20:45, 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. > > 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(-) > > 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; > } > Applied to disco/master-next branch. Thanks, Kleber
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; }
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(-)