[SRU,FOCAL,2/2] UBUNTU: SAUCE: shiftfs: prevent ESTALE for LOOKUP_JUMP lookups

BugLink: https://bugs.launchpad.net/bugs/1872757

Users reported that creating temporary files shiftfs reports ESTALE.
This can be reproduced via:

import tempfile
import os

def test():
    with tempfile.TemporaryFile() as fd:
        fd.write("data".encode('utf-8'))
        # re-open the file to get a read-only file descriptor
        return open(f"/proc/self/fd/{fd.fileno()}", "r")

def main():
   fd = test()
   fd.close()

if __name__ == "__main__":
    main()

a similar issue was reported here:
https://github.com/systemd/systemd/issues/14861

Our revalidate methods were very opinionated about whether or not a
lower dentry was valid especially when it became unlinked we simply
invalidated the lower dentry which caused above bug to surface. 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.

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 | 4 ----
 1 file changed, 4 deletions(-)

On Tue, Jun 23, 2020 at 07:46:16PM +0200, Christian Brauner wrote:
> BugLink: https://bugs.launchpad.net/bugs/1872757
> 
> Users reported that creating temporary files shiftfs reports ESTALE.
> This can be reproduced via:
> 
> import tempfile
> import os
> 
> def test():
>     with tempfile.TemporaryFile() as fd:
>         fd.write("data".encode('utf-8'))
>         # re-open the file to get a read-only file descriptor
>         return open(f"/proc/self/fd/{fd.fileno()}", "r")
> 
> def main():
>    fd = test()
>    fd.close()
> 
> if __name__ == "__main__":
>     main()
> 
> a similar issue was reported here:
> https://github.com/systemd/systemd/issues/14861
> 
> Our revalidate methods were very opinionated about whether or not a
> lower dentry was valid especially when it became unlinked we simply
> invalidated the lower dentry which caused above bug to surface. 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.
> 
> 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>

This looks okay to me, and no other stacked filesystem checks i_nlink of
the lower filesystem inode.

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

On Tue, Jun 23, 2020 at 01:43:36PM -0500, Seth Forshee wrote:
> On Tue, Jun 23, 2020 at 07:46:16PM +0200, Christian Brauner wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1872757
> > 
> > Users reported that creating temporary files shiftfs reports ESTALE.
> > This can be reproduced via:
> > 
> > import tempfile
> > import os
> > 
> > def test():
> >     with tempfile.TemporaryFile() as fd:
> >         fd.write("data".encode('utf-8'))
> >         # re-open the file to get a read-only file descriptor
> >         return open(f"/proc/self/fd/{fd.fileno()}", "r")
> > 
> > def main():
> >    fd = test()
> >    fd.close()
> > 
> > if __name__ == "__main__":
> >     main()
> > 
> > a similar issue was reported here:
> > https://github.com/systemd/systemd/issues/14861
> > 
> > Our revalidate methods were very opinionated about whether or not a
> > lower dentry was valid especially when it became unlinked we simply
> > invalidated the lower dentry which caused above bug to surface. 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.
> > 
> > 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>
> 
> This looks okay to me, and no other stacked filesystem checks i_nlink of
> the lower filesystem inode.

There's still a very subtle issue here namely that in some circumstances
editing the lower fs can still cause the shiftfs and lower cache to get
out of sync. I'm not yet sure how to fix this and there's no other fs
that has a similer task. I need to find some time to re-vamp the
upstream in-vfs patches I sent last cycle. But likely we'll have another
discussion around this at Plumbers.

Thanks!
Christian