| Message ID | 20200414201737.173672-1-christian.brauner@ubuntu.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,UNSTABLE/BIONIC/XENIAL] UBUNTU: SAUCE: shiftfs: remove custom tmpfile method | expand |
Acked-by: Kamal Mostafa <kamal@canonical.com> -Kamal On Tue, Apr 14, 2020 at 10:17:37PM +0200, Christian Brauner wrote: > BugLink: https://bugs.launchpad.net/bugs/1872757 > > Our revalidate methods were very opinionated about whether or not a > dentry was valid when we really should've just let the underlay tell us > what's what. This has led to bugs where a ESTALE was returned for e.g. > temporary files that were created and directly re-opened afterwards > through /proc/<pid>/fd/<nr-of-deleted-file>. When a file is re-opened > through /proc/<pid>/fd/<nr> LOOKUP_JUMP is set and the vfs will > revalidate via d_weak_revalidate(). Since the file has been unhashed or > even already gone negative we'd fail the open when we should've > succeeded. > > I had also foolishly provided a .tmpfile method which so far only has > caused us trouble and which was involved in the ESTALE problem. If we > really need this then we can reimplement it properly but I doubt it. > Remove it for now. > > Reported-by: Christian Kellner <ckellner@redhat.com> > Reported-by: Evgeny Vereshchagin >evvers@ya.ru> > Cc: Seth Forshee <seth.forshee@canonical.com> > Link: https://github.com/systemd/systemd/issues/14861 > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> > --- > fs/shiftfs.c | 45 +++++++++++++-------------------------------- > 1 file changed, 13 insertions(+), 32 deletions(-) > > diff --git a/fs/shiftfs.c b/fs/shiftfs.c > index 5c39529d0a17..39b878a6f820 100644 > --- a/fs/shiftfs.c > +++ b/fs/shiftfs.c > @@ -240,19 +240,17 @@ static int shiftfs_d_weak_revalidate(struct dentry *dentry, unsigned int flags) > int err = 1; > struct dentry *lowerd = dentry->d_fsdata; > > - if (d_is_negative(lowerd) != d_is_negative(dentry)) > - return 0; > - > - if ((lowerd->d_flags & DCACHE_OP_WEAK_REVALIDATE)) > + 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; > @@ -263,23 +261,25 @@ 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,24 +736,6 @@ 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; > @@ -1020,7 +1002,6 @@ 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: 1ff0ece4a5b555e1a72ec8d030b9410761174598 > -- > 2.26.0 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On Tue, Apr 14, 2020 at 10:17:37PM +0200, Christian Brauner wrote: > BugLink: https://bugs.launchpad.net/bugs/1872757 > > Our revalidate methods were very opinionated about whether or not a > dentry was valid when we really should've just let the underlay tell us > what's what. This has led to bugs where a ESTALE was returned for e.g. > temporary files that were created and directly re-opened afterwards > through /proc/<pid>/fd/<nr-of-deleted-file>. When a file is re-opened > through /proc/<pid>/fd/<nr> LOOKUP_JUMP is set and the vfs will > revalidate via d_weak_revalidate(). Since the file has been unhashed or > even already gone negative we'd fail the open when we should've > succeeded. > > I had also foolishly provided a .tmpfile method which so far only has > caused us trouble and which was involved in the ESTALE problem. If we > really need this then we can reimplement it properly but I doubt it. > Remove it for now. > > Reported-by: Christian Kellner <ckellner@redhat.com> > Reported-by: Evgeny Vereshchagin >evvers@ya.ru> > Cc: Seth Forshee <seth.forshee@canonical.com> > Link: https://github.com/systemd/systemd/issues/14861 > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> Applied to unstable/master, thanks!
On Fri, May 01, 2020 at 08:59:17PM -0500, Seth Forshee wrote: > On Tue, Apr 14, 2020 at 10:17:37PM +0200, Christian Brauner wrote: > > BugLink: https://bugs.launchpad.net/bugs/1872757 > > > > Our revalidate methods were very opinionated about whether or not a > > dentry was valid when we really should've just let the underlay tell us > > what's what. This has led to bugs where a ESTALE was returned for e.g. > > temporary files that were created and directly re-opened afterwards > > through /proc/<pid>/fd/<nr-of-deleted-file>. When a file is re-opened > > through /proc/<pid>/fd/<nr> LOOKUP_JUMP is set and the vfs will > > revalidate via d_weak_revalidate(). Since the file has been unhashed or > > even already gone negative we'd fail the open when we should've > > succeeded. > > > > I had also foolishly provided a .tmpfile method which so far only has > > caused us trouble and which was involved in the ESTALE problem. If we > > really need this then we can reimplement it properly but I doubt it. > > Remove it for now. > > > > Reported-by: Christian Kellner <ckellner@redhat.com> > > Reported-by: Evgeny Vereshchagin >evvers@ya.ru> > > Cc: Seth Forshee <seth.forshee@canonical.com> > > Link: https://github.com/systemd/systemd/issues/14861 > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> > > Applied to unstable/master, thanks! Oops, replied to the wrong message. The one I actually applied was "UBUNTU: SAUCE: shiftfs: fix dentry revalidation."
diff --git a/fs/shiftfs.c b/fs/shiftfs.c index 5c39529d0a17..39b878a6f820 100644 --- a/fs/shiftfs.c +++ b/fs/shiftfs.c @@ -240,19 +240,17 @@ static int shiftfs_d_weak_revalidate(struct dentry *dentry, unsigned int flags) int err = 1; struct dentry *lowerd = dentry->d_fsdata; - if (d_is_negative(lowerd) != d_is_negative(dentry)) - return 0; - - if ((lowerd->d_flags & DCACHE_OP_WEAK_REVALIDATE)) + 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; @@ -263,23 +261,25 @@ 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,24 +736,6 @@ 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; @@ -1020,7 +1002,6 @@ 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 = {
BugLink: https://bugs.launchpad.net/bugs/1872757 Our revalidate methods were very opinionated about whether or not a dentry was valid when we really should've just let the underlay tell us what's what. This has led to bugs where a ESTALE was returned for e.g. temporary files that were created and directly re-opened afterwards through /proc/<pid>/fd/<nr-of-deleted-file>. When a file is re-opened through /proc/<pid>/fd/<nr> LOOKUP_JUMP is set and the vfs will revalidate via d_weak_revalidate(). Since the file has been unhashed or even already gone negative we'd fail the open when we should've succeeded. I had also foolishly provided a .tmpfile method which so far only has caused us trouble and which was involved in the ESTALE problem. If we really need this then we can reimplement it properly but I doubt it. Remove it for now. Reported-by: Christian Kellner <ckellner@redhat.com> Reported-by: Evgeny Vereshchagin >evvers@ya.ru> Cc: Seth Forshee <seth.forshee@canonical.com> Link: https://github.com/systemd/systemd/issues/14861 Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> --- fs/shiftfs.c | 45 +++++++++++++-------------------------------- 1 file changed, 13 insertions(+), 32 deletions(-) base-commit: 1ff0ece4a5b555e1a72ec8d030b9410761174598