[CRIU] [SRU][F][G][H][PATCH v2] UBUNTU: SAUCE: overlayfs: fix incorrect mnt_id of files opened from map_files

[Christian Brauner]

On Tue, Apr 27, 2021 at 12:03:17PM +0200, Krzysztof Kozlowski wrote:
> On 27/04/2021 11:46, Christian Brauner wrote:
> > On Mon, Apr 26, 2021 at 11:11:21AM +0300, alexander at mihalicyn.com wrote:
> >> From: Alexander Mikhalitsyn <alexander at mihalicyn.com>
> >>
> >> BugLink: https://bugs.launchpad.net/bugs/1857257
> >>
> >> The hack was introduced in ("UBUNTU: SAUCE: overlayfs: allow with
> >> shiftfs as underlay") and it broke checkpoint/restore of docker
> >> contains:
> >> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1857257
> >>
> >> The following script can be used to trigger the issue:
> >>   #!/bin/bash
> >>
> >>   cat > test.py << EOF
> >>   import sys
> >>
> >>   f = open("/proc/self/maps")
> >>
> >>   for l in f.readlines():
> >>     if "python" not in l:
> >>       continue
> >>     print(l)
> >>     s = l.split()
> >>     start, end = s[0].split("-")
> >>     fname = s[-1]
> >>     print(start, end, fname)
> >>     break
> >>   else:
> >>     sys.exit(1)
> >>
> >>   test_file1 = open(fname)
> >>   test_file2 = open("/proc/self/map_files/%s-%s" % (start, end))
> >>
> >>   fdinfo1 = open("/proc/self/fdinfo/%d" % test_file1.fileno()).read()
> >>   fdinfo2 = open("/proc/self/fdinfo/%d" % test_file2.fileno()).read()
> >>
> >>   if fdinfo1 != fdinfo2:
> >>     print("FAIL")
> >>     print(test_file1)
> >>     print(fdinfo1)
> >>     print(test_file2)
> >>     print(fdinfo2)
> >>     sys.exit(1)
> >>   print("PASS")
> >>   EOF
> >>   sudo docker run -it --privileged --rm -v `pwd`:/mnt python python /mnt/test.py
> >>
> >> Thanks to Andrei Vagin for the reproducer and investigation of this problem.
> >>
> >> Cc: Andrei Vagin <avagin at gmail.com>
> >> Cc: Adrian Reber <areber at redhat.com>
> >> Cc: Christian Brauner <christian.brauner at ubuntu.com>
> >> Cc: Stefan Bader <stefan.bader at canonical.com>
> >> Cc: Kleber Sacilotto de Souza <kleber.souza at canonical.com>
> >> Cc: Krzysztof Kozlowski <krzysztof.kozlowski at canonical.com>
> >>
> >> Fixes: d24b8a5 ("UBUNTU: SAUCE: overlayfs: allow with shiftfs as underlay")
> >> Signed-off-by: Alexander Mikhalitsyn <alexander at mihalicyn.com>
> >> ---
> > 
> > Hey,
> > 
> > Thanks for the patch!
> > Fwiw, Andrei already tried to fix this a while ago but it caused another
> > regression which forced us to revert the fix for this issue.
> 
> Did you mean you tried this patch and it caused regressions?

No, we had an earlier fix for this issue which didn't depend on aufs
that Andrei sent:

commit 9ac77e721a0edb87cbbe95f9e75d8e27d96cb051
Author: Kleber Sacilotto de Souza <kleber.souza at canonical.com>
Date:   Thu May 21 12:55:43 2020 +0200

    Revert "UBUNTU: SAUCE: overlayfs: use shiftfs hacks only with shiftfs as underlay"
    
    BugLink: https://bugs.launchpad.net/bugs/1879690
    
    This reverts commit 6f18a8434050333afc80b5bfce2e0e994c86b790.
    
    The change applied for LP: #1857257 and its followup fix LP: #1876645
    introduced a regression on overlayfs. Revert these commits for now.
    
    Signed-off-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>
    Acked-by: Colin Ian King <colin.king at canonical.com>
    Acked-by: Khalid Elmously <khalid.elmously at canonical.com>
    Signed-off-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>

Shifts is trying to overcome a shortcoming in overlayfs itself where
overlayfs is constructing a fake path where fake path means mnt and
dentry from the overlay but inode from another (lower or upper)
filesystem which is a hack to workaround related issues to the one we're
doing here.

> 
> > 
> >>  fs/overlayfs/file.c | 29 +++++++++++++++++++++++++++++
> >>  1 file changed, 29 insertions(+)
> >>
> >> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> >> index 0d3ea0cf3e98..5fa520d0798e 100644
> >> --- a/fs/overlayfs/file.c
> >> +++ b/fs/overlayfs/file.c
> >> @@ -325,6 +325,18 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> >>  	return ret;
> >>  }
> >>  
> >> +/* handle vma->vm_prfile */
> >> +static void ovl_vm_prfile_set(struct vm_area_struct *vma,
> >> +			      struct file *file)
> >> +{
> >> +	get_file(file);
> >> +	vma->vm_prfile = file;
> >> +#ifndef CONFIG_MMU
> >> +	get_file(file);
> >> +	vma->vm_region->vm_prfile = file;
> >> +#endif
> >> +}
> > 
> > I'm confused struct vm_area_struct doesn't have a vm_prfile entry. Can
> > you explain, please?
> 
> It has. The aufs patchset brought it and basically this patch depends on
> aufs.

I meant upstream but good to know.
Btw, why do we still carry aufs?
This is not my business but I fear this will mean you're committing to
carrying either aufs or a specific aufs patch with Ubuntu for the sake
of overlayfs even if you wanted to drop aufs in the future.