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

[Alexander Mihalicyn]

On Tue, Apr 27, 2021 at 1:28 PM Christian Brauner
<christian.brauner at ubuntu.com> wrote:
>
> 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.

If the aufs will be dropped from the Ubuntu kernel, this part with
vma_pr_or_file() and additional
vma field can be safely saved in the kernel.
It's an independent part (mm/prfile.c + several small changes in
proc). So, it's safe to keep
it in the kernel for overlayfs/shiftfs needs.

It's also important to notice that this problem with incorrect
map_files mnt_id is also valid
for current shiftfs implementation. If we decide to take this
particular patch for overlayfs,
I will prepare an analogical fix for the shiftfs.