diff mbox series

[SRU,FOCAL,1/2] UBUNTU: SAUCE: Revert "UBUNTU: SAUCE: shiftfs: fix dentry revalidation"

Message ID 20200623174616.972066-1-christian.brauner@ubuntu.com
State New
Headers show
Series [SRU,FOCAL,1/2] UBUNTU: SAUCE: Revert "UBUNTU: SAUCE: shiftfs: fix dentry revalidation" | expand

Commit Message

Christian Brauner June 23, 2020, 5:46 p.m. UTC
BugLink: https://bugs.launchpad.net/bugs/1884767

With patch cfaa482afb97 ("UBUNTU: SAUCE: shiftfs: fix dentry revalidation")
we tried to fix
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1872757 but it
introduces a regression for shiftfs on btrfs users. Creating a btrfs
subvolume, deleting it, and then trying to recreate it will cause EEXIST
to be returned or the file be left in a half-created state. Faulty
behavior such as this can be reproduced via:

btrfs subvolume create my-subvol
ls -al my-subvol
btrfs subvolume delete my-subvol

We thus need to revert this change restoring the old behavior.This will
briefly resurface https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1872757
which is fixed in a follow-up patch on top of this revert. We simply
split out the minimal fix into a separate tiny patch.

This reverts commit cfaa482afb97e3c05d020af80b897b061109d51f.

Cc: Seth Forshee <seth.forshee@canonical.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 fs/shiftfs.c | 45 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 13 deletions(-)


base-commit: 51ee04d9d3b464e9aa8509013779491f0b001ebc

Comments

Christian Brauner June 23, 2020, 5:48 p.m. UTC | #1
On Tue, Jun 23, 2020 at 07:46:15PM +0200, Christian Brauner wrote:
> BugLink: https://bugs.launchpad.net/bugs/1884767
> 
> With patch cfaa482afb97 ("UBUNTU: SAUCE: shiftfs: fix dentry revalidation")
> we tried to fix
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1872757 but it
> introduces a regression for shiftfs on btrfs users. Creating a btrfs
> subvolume, deleting it, and then trying to recreate it will cause EEXIST
> to be returned or the file be left in a half-created state. Faulty
> behavior such as this can be reproduced via:
> 
> btrfs subvolume create my-subvol
> ls -al my-subvol
> btrfs subvolume delete my-subvol
> 
> We thus need to revert this change restoring the old behavior.This will
> briefly resurface https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1872757
> which is fixed in a follow-up patch on top of this revert. We simply
> split out the minimal fix into a separate tiny patch.
> 
> This reverts commit cfaa482afb97e3c05d020af80b897b061109d51f.
> 
> Cc: Seth Forshee <seth.forshee@canonical.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---

The patch series has been tested against:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1872757
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1879196
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1860041
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1879688

Fixes for eoan and unstable will come separately. Disco doesn't have the
problematic patch backported afaict.

Thanks!
Christian

>  fs/shiftfs.c | 45 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/shiftfs.c b/fs/shiftfs.c
> index f5c709805fb8..5d88193b41db 100644
> --- a/fs/shiftfs.c
> +++ b/fs/shiftfs.c
> @@ -240,17 +240,19 @@ static int shiftfs_d_weak_revalidate(struct dentry *dentry, unsigned int flags)
>  	int err = 1;
>  	struct dentry *lowerd = dentry->d_fsdata;
>  
> -	if (lowerd->d_flags & DCACHE_OP_WEAK_REVALIDATE) {
> +	if (d_is_negative(lowerd) != d_is_negative(dentry))
> +		return 0;
> +
> +	if ((lowerd->d_flags & DCACHE_OP_WEAK_REVALIDATE))
>  		err = lowerd->d_op->d_weak_revalidate(lowerd, flags);
> -		if (err < 0)
> -			return err;
> -	}
>  
>  	if (d_really_is_positive(dentry)) {
>  		struct inode *inode = d_inode(dentry);
>  		struct inode *loweri = d_inode(lowerd);
>  
>  		shiftfs_copyattr(loweri, inode);
> +		if (!inode->i_nlink)
> +			err = 0;
>  	}
>  
>  	return err;
> @@ -261,25 +263,23 @@ static int shiftfs_d_revalidate(struct dentry *dentry, unsigned int flags)
>  	int err = 1;
>  	struct dentry *lowerd = dentry->d_fsdata;
>  
> +	if (d_unhashed(lowerd) ||
> +	    ((d_is_negative(lowerd) != d_is_negative(dentry))))
> +		return 0;
> +
>  	if (flags & LOOKUP_RCU)
>  		return -ECHILD;
>  
> -	if (lowerd->d_flags & DCACHE_OP_REVALIDATE) {
> +	if ((lowerd->d_flags & DCACHE_OP_REVALIDATE))
>  		err = lowerd->d_op->d_revalidate(lowerd, flags);
> -		if (err < 0)
> -			return err;
> -		if (!err) {
> -			if (!(flags & LOOKUP_RCU))
> -				d_invalidate(lowerd);
> -			return -ESTALE;
> -		}
> -	}
>  
>  	if (d_really_is_positive(dentry)) {
>  		struct inode *inode = d_inode(dentry);
>  		struct inode *loweri = d_inode(lowerd);
>  
>  		shiftfs_copyattr(loweri, inode);
> +		if (!inode->i_nlink)
> +			err = 0;
>  	}
>  
>  	return err;
> @@ -736,6 +736,24 @@ static int shiftfs_fiemap(struct inode *inode,
>  	return err;
>  }
>  
> +static int shiftfs_tmpfile(struct inode *dir, struct dentry *dentry,
> +			   umode_t mode)
> +{
> +	int err;
> +	const struct cred *oldcred;
> +	struct dentry *lowerd = dentry->d_fsdata;
> +	struct inode *loweri = dir->i_private;
> +
> +	if (!loweri->i_op->tmpfile)
> +		return -EOPNOTSUPP;
> +
> +	oldcred = shiftfs_override_creds(dir->i_sb);
> +	err = loweri->i_op->tmpfile(loweri, lowerd, mode);
> +	revert_creds(oldcred);
> +
> +	return err;
> +}
> +
>  static int shiftfs_setattr(struct dentry *dentry, struct iattr *attr)
>  {
>  	struct dentry *lowerd = dentry->d_fsdata;
> @@ -1002,6 +1020,7 @@ static const struct inode_operations shiftfs_file_inode_operations = {
>  	.listxattr	= shiftfs_listxattr,
>  	.permission	= shiftfs_permission,
>  	.setattr	= shiftfs_setattr,
> +	.tmpfile	= shiftfs_tmpfile,
>  };
>  
>  static const struct inode_operations shiftfs_special_inode_operations = {
> 
> base-commit: 51ee04d9d3b464e9aa8509013779491f0b001ebc
> -- 
> 2.27.0
>
Seth Forshee June 23, 2020, 6:42 p.m. UTC | #2
On Tue, Jun 23, 2020 at 07:46:15PM +0200, Christian Brauner wrote:
> BugLink: https://bugs.launchpad.net/bugs/1884767
> 
> With patch cfaa482afb97 ("UBUNTU: SAUCE: shiftfs: fix dentry revalidation")
> we tried to fix
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1872757 but it
> introduces a regression for shiftfs on btrfs users. Creating a btrfs
> subvolume, deleting it, and then trying to recreate it will cause EEXIST
> to be returned or the file be left in a half-created state. Faulty
> behavior such as this can be reproduced via:
> 
> btrfs subvolume create my-subvol
> ls -al my-subvol
> btrfs subvolume delete my-subvol
> 
> We thus need to revert this change restoring the old behavior.This will
> briefly resurface https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1872757
> which is fixed in a follow-up patch on top of this revert. We simply
> split out the minimal fix into a separate tiny patch.
> 
> This reverts commit cfaa482afb97e3c05d020af80b897b061109d51f.
> 
> Cc: Seth Forshee <seth.forshee@canonical.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

Acked-by: Seth Forshee <seth.forshee@canonical.com>
Stefan Bader June 24, 2020, 7:23 a.m. UTC | #3
On 23.06.20 19:46, Christian Brauner wrote:
> BugLink: https://bugs.launchpad.net/bugs/1884767
> 
> With patch cfaa482afb97 ("UBUNTU: SAUCE: shiftfs: fix dentry revalidation")
> we tried to fix
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1872757 but it
> introduces a regression for shiftfs on btrfs users. Creating a btrfs
> subvolume, deleting it, and then trying to recreate it will cause EEXIST
> to be returned or the file be left in a half-created state. Faulty
> behavior such as this can be reproduced via:
> 
> btrfs subvolume create my-subvol
> ls -al my-subvol
> btrfs subvolume delete my-subvol
> 
> We thus need to revert this change restoring the old behavior.This will
> briefly resurface https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1872757
> which is fixed in a follow-up patch on top of this revert. We simply
> split out the minimal fix into a separate tiny patch.
> 
> This reverts commit cfaa482afb97e3c05d020af80b897b061109d51f.
> 
> Cc: Seth Forshee <seth.forshee@canonical.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---

As well for the second patch, though I must say I am not very optimistic about
not seeing that one as a revert in the future... Not technically based but
shiftfs related SRU's seem to follow a pattern of going back and forth...

-Stefan

>  fs/shiftfs.c | 45 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/shiftfs.c b/fs/shiftfs.c
> index f5c709805fb8..5d88193b41db 100644
> --- a/fs/shiftfs.c
> +++ b/fs/shiftfs.c
> @@ -240,17 +240,19 @@ static int shiftfs_d_weak_revalidate(struct dentry *dentry, unsigned int flags)
>  	int err = 1;
>  	struct dentry *lowerd = dentry->d_fsdata;
>  
> -	if (lowerd->d_flags & DCACHE_OP_WEAK_REVALIDATE) {
> +	if (d_is_negative(lowerd) != d_is_negative(dentry))
> +		return 0;
> +
> +	if ((lowerd->d_flags & DCACHE_OP_WEAK_REVALIDATE))
>  		err = lowerd->d_op->d_weak_revalidate(lowerd, flags);
> -		if (err < 0)
> -			return err;
> -	}
>  
>  	if (d_really_is_positive(dentry)) {
>  		struct inode *inode = d_inode(dentry);
>  		struct inode *loweri = d_inode(lowerd);
>  
>  		shiftfs_copyattr(loweri, inode);
> +		if (!inode->i_nlink)
> +			err = 0;
>  	}
>  
>  	return err;
> @@ -261,25 +263,23 @@ static int shiftfs_d_revalidate(struct dentry *dentry, unsigned int flags)
>  	int err = 1;
>  	struct dentry *lowerd = dentry->d_fsdata;
>  
> +	if (d_unhashed(lowerd) ||
> +	    ((d_is_negative(lowerd) != d_is_negative(dentry))))
> +		return 0;
> +
>  	if (flags & LOOKUP_RCU)
>  		return -ECHILD;
>  
> -	if (lowerd->d_flags & DCACHE_OP_REVALIDATE) {
> +	if ((lowerd->d_flags & DCACHE_OP_REVALIDATE))
>  		err = lowerd->d_op->d_revalidate(lowerd, flags);
> -		if (err < 0)
> -			return err;
> -		if (!err) {
> -			if (!(flags & LOOKUP_RCU))
> -				d_invalidate(lowerd);
> -			return -ESTALE;
> -		}
> -	}
>  
>  	if (d_really_is_positive(dentry)) {
>  		struct inode *inode = d_inode(dentry);
>  		struct inode *loweri = d_inode(lowerd);
>  
>  		shiftfs_copyattr(loweri, inode);
> +		if (!inode->i_nlink)
> +			err = 0;
>  	}
>  
>  	return err;
> @@ -736,6 +736,24 @@ static int shiftfs_fiemap(struct inode *inode,
>  	return err;
>  }
>  
> +static int shiftfs_tmpfile(struct inode *dir, struct dentry *dentry,
> +			   umode_t mode)
> +{
> +	int err;
> +	const struct cred *oldcred;
> +	struct dentry *lowerd = dentry->d_fsdata;
> +	struct inode *loweri = dir->i_private;
> +
> +	if (!loweri->i_op->tmpfile)
> +		return -EOPNOTSUPP;
> +
> +	oldcred = shiftfs_override_creds(dir->i_sb);
> +	err = loweri->i_op->tmpfile(loweri, lowerd, mode);
> +	revert_creds(oldcred);
> +
> +	return err;
> +}
> +
>  static int shiftfs_setattr(struct dentry *dentry, struct iattr *attr)
>  {
>  	struct dentry *lowerd = dentry->d_fsdata;
> @@ -1002,6 +1020,7 @@ static const struct inode_operations shiftfs_file_inode_operations = {
>  	.listxattr	= shiftfs_listxattr,
>  	.permission	= shiftfs_permission,
>  	.setattr	= shiftfs_setattr,
> +	.tmpfile	= shiftfs_tmpfile,
>  };
>  
>  static const struct inode_operations shiftfs_special_inode_operations = {
> 
> base-commit: 51ee04d9d3b464e9aa8509013779491f0b001ebc
>
Christian Brauner June 24, 2020, 8:03 a.m. UTC | #4
On Wed, Jun 24, 2020 at 09:23:15AM +0200, Stefan Bader wrote:
> On 23.06.20 19:46, Christian Brauner wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1884767
> > 
> > With patch cfaa482afb97 ("UBUNTU: SAUCE: shiftfs: fix dentry revalidation")
> > we tried to fix
> > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1872757 but it
> > introduces a regression for shiftfs on btrfs users. Creating a btrfs
> > subvolume, deleting it, and then trying to recreate it will cause EEXIST
> > to be returned or the file be left in a half-created state. Faulty
> > behavior such as this can be reproduced via:
> > 
> > btrfs subvolume create my-subvol
> > ls -al my-subvol
> > btrfs subvolume delete my-subvol
> > 
> > We thus need to revert this change restoring the old behavior.This will
> > briefly resurface https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1872757
> > which is fixed in a follow-up patch on top of this revert. We simply
> > split out the minimal fix into a separate tiny patch.
> > 
> > This reverts commit cfaa482afb97e3c05d020af80b897b061109d51f.
> > 
> > Cc: Seth Forshee <seth.forshee@canonical.com>
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> Acked-by: Stefan Bader <stefan.bader@canonical.com>
> > ---
> 
> As well for the second patch, though I must say I am not very optimistic about

Thanks!

> not seeing that one as a revert in the future... Not technically based but
> shiftfs related SRU's seem to follow a pattern of going back and forth...

Not particularly, especially when compared to other fs (with or without
similar behavior of having to keep 2 caches in sync). overlayfs alone
has ~15 reverts including revert(revert) which we don't have. So far we
have 1 revert and the change that fixes the bug that it was designed to
fix has been moved into 2/2.

Christian

> 
> -Stefan
> 
> >  fs/shiftfs.c | 45 ++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 32 insertions(+), 13 deletions(-)
> > 
> > diff --git a/fs/shiftfs.c b/fs/shiftfs.c
> > index f5c709805fb8..5d88193b41db 100644
> > --- a/fs/shiftfs.c
> > +++ b/fs/shiftfs.c
> > @@ -240,17 +240,19 @@ static int shiftfs_d_weak_revalidate(struct dentry *dentry, unsigned int flags)
> >  	int err = 1;
> >  	struct dentry *lowerd = dentry->d_fsdata;
> >  
> > -	if (lowerd->d_flags & DCACHE_OP_WEAK_REVALIDATE) {
> > +	if (d_is_negative(lowerd) != d_is_negative(dentry))
> > +		return 0;
> > +
> > +	if ((lowerd->d_flags & DCACHE_OP_WEAK_REVALIDATE))
> >  		err = lowerd->d_op->d_weak_revalidate(lowerd, flags);
> > -		if (err < 0)
> > -			return err;
> > -	}
> >  
> >  	if (d_really_is_positive(dentry)) {
> >  		struct inode *inode = d_inode(dentry);
> >  		struct inode *loweri = d_inode(lowerd);
> >  
> >  		shiftfs_copyattr(loweri, inode);
> > +		if (!inode->i_nlink)
> > +			err = 0;
> >  	}
> >  
> >  	return err;
> > @@ -261,25 +263,23 @@ static int shiftfs_d_revalidate(struct dentry *dentry, unsigned int flags)
> >  	int err = 1;
> >  	struct dentry *lowerd = dentry->d_fsdata;
> >  
> > +	if (d_unhashed(lowerd) ||
> > +	    ((d_is_negative(lowerd) != d_is_negative(dentry))))
> > +		return 0;
> > +
> >  	if (flags & LOOKUP_RCU)
> >  		return -ECHILD;
> >  
> > -	if (lowerd->d_flags & DCACHE_OP_REVALIDATE) {
> > +	if ((lowerd->d_flags & DCACHE_OP_REVALIDATE))
> >  		err = lowerd->d_op->d_revalidate(lowerd, flags);
> > -		if (err < 0)
> > -			return err;
> > -		if (!err) {
> > -			if (!(flags & LOOKUP_RCU))
> > -				d_invalidate(lowerd);
> > -			return -ESTALE;
> > -		}
> > -	}
> >  
> >  	if (d_really_is_positive(dentry)) {
> >  		struct inode *inode = d_inode(dentry);
> >  		struct inode *loweri = d_inode(lowerd);
> >  
> >  		shiftfs_copyattr(loweri, inode);
> > +		if (!inode->i_nlink)
> > +			err = 0;
> >  	}
> >  
> >  	return err;
> > @@ -736,6 +736,24 @@ static int shiftfs_fiemap(struct inode *inode,
> >  	return err;
> >  }
> >  
> > +static int shiftfs_tmpfile(struct inode *dir, struct dentry *dentry,
> > +			   umode_t mode)
> > +{
> > +	int err;
> > +	const struct cred *oldcred;
> > +	struct dentry *lowerd = dentry->d_fsdata;
> > +	struct inode *loweri = dir->i_private;
> > +
> > +	if (!loweri->i_op->tmpfile)
> > +		return -EOPNOTSUPP;
> > +
> > +	oldcred = shiftfs_override_creds(dir->i_sb);
> > +	err = loweri->i_op->tmpfile(loweri, lowerd, mode);
> > +	revert_creds(oldcred);
> > +
> > +	return err;
> > +}
> > +
> >  static int shiftfs_setattr(struct dentry *dentry, struct iattr *attr)
> >  {
> >  	struct dentry *lowerd = dentry->d_fsdata;
> > @@ -1002,6 +1020,7 @@ static const struct inode_operations shiftfs_file_inode_operations = {
> >  	.listxattr	= shiftfs_listxattr,
> >  	.permission	= shiftfs_permission,
> >  	.setattr	= shiftfs_setattr,
> > +	.tmpfile	= shiftfs_tmpfile,
> >  };
> >  
> >  static const struct inode_operations shiftfs_special_inode_operations = {
> > 
> > base-commit: 51ee04d9d3b464e9aa8509013779491f0b001ebc
> > 
> 
>
diff mbox series

Patch

diff --git a/fs/shiftfs.c b/fs/shiftfs.c
index f5c709805fb8..5d88193b41db 100644
--- a/fs/shiftfs.c
+++ b/fs/shiftfs.c
@@ -240,17 +240,19 @@  static int shiftfs_d_weak_revalidate(struct dentry *dentry, unsigned int flags)
 	int err = 1;
 	struct dentry *lowerd = dentry->d_fsdata;
 
-	if (lowerd->d_flags & DCACHE_OP_WEAK_REVALIDATE) {
+	if (d_is_negative(lowerd) != d_is_negative(dentry))
+		return 0;
+
+	if ((lowerd->d_flags & DCACHE_OP_WEAK_REVALIDATE))
 		err = lowerd->d_op->d_weak_revalidate(lowerd, flags);
-		if (err < 0)
-			return err;
-	}
 
 	if (d_really_is_positive(dentry)) {
 		struct inode *inode = d_inode(dentry);
 		struct inode *loweri = d_inode(lowerd);
 
 		shiftfs_copyattr(loweri, inode);
+		if (!inode->i_nlink)
+			err = 0;
 	}
 
 	return err;
@@ -261,25 +263,23 @@  static int shiftfs_d_revalidate(struct dentry *dentry, unsigned int flags)
 	int err = 1;
 	struct dentry *lowerd = dentry->d_fsdata;
 
+	if (d_unhashed(lowerd) ||
+	    ((d_is_negative(lowerd) != d_is_negative(dentry))))
+		return 0;
+
 	if (flags & LOOKUP_RCU)
 		return -ECHILD;
 
-	if (lowerd->d_flags & DCACHE_OP_REVALIDATE) {
+	if ((lowerd->d_flags & DCACHE_OP_REVALIDATE))
 		err = lowerd->d_op->d_revalidate(lowerd, flags);
-		if (err < 0)
-			return err;
-		if (!err) {
-			if (!(flags & LOOKUP_RCU))
-				d_invalidate(lowerd);
-			return -ESTALE;
-		}
-	}
 
 	if (d_really_is_positive(dentry)) {
 		struct inode *inode = d_inode(dentry);
 		struct inode *loweri = d_inode(lowerd);
 
 		shiftfs_copyattr(loweri, inode);
+		if (!inode->i_nlink)
+			err = 0;
 	}
 
 	return err;
@@ -736,6 +736,24 @@  static int shiftfs_fiemap(struct inode *inode,
 	return err;
 }
 
+static int shiftfs_tmpfile(struct inode *dir, struct dentry *dentry,
+			   umode_t mode)
+{
+	int err;
+	const struct cred *oldcred;
+	struct dentry *lowerd = dentry->d_fsdata;
+	struct inode *loweri = dir->i_private;
+
+	if (!loweri->i_op->tmpfile)
+		return -EOPNOTSUPP;
+
+	oldcred = shiftfs_override_creds(dir->i_sb);
+	err = loweri->i_op->tmpfile(loweri, lowerd, mode);
+	revert_creds(oldcred);
+
+	return err;
+}
+
 static int shiftfs_setattr(struct dentry *dentry, struct iattr *attr)
 {
 	struct dentry *lowerd = dentry->d_fsdata;
@@ -1002,6 +1020,7 @@  static const struct inode_operations shiftfs_file_inode_operations = {
 	.listxattr	= shiftfs_listxattr,
 	.permission	= shiftfs_permission,
 	.setattr	= shiftfs_setattr,
+	.tmpfile	= shiftfs_tmpfile,
 };
 
 static const struct inode_operations shiftfs_special_inode_operations = {