[SRU][F][PATCH 1/1] UBUNTU: SAUCE: overlayfs: fix reference count mismatch

[Andrea Righi]

On Thu, Jun 22, 2023 at 12:14:26PM +0200, Aleksandr Mikhalitsyn wrote:
> On Thu, Jun 22, 2023 at 11:38 AM Andrea Righi
> <andrea.righi at canonical.com> wrote:
> >
> > BugLink: https://bugs.launchpad.net/bugs/2016398
> >
> > Opened files reported in /proc/pid/map_files can be shows with the wrong
> > mount point using overlayfs with filesystem namspaces.
> >
> > This incorrect behavior is fixed:
> >
> >   UBUNTU: SAUCE: overlayfs: fix incorrect mnt_id of files opened from map_files
> >
> > However, the fix introduced a new regression, the reference to the
> > original file stored in vma->vm_prfile is not properly released when
> > vma->vm_prfile is replaced with a new file.
> >
> > This can cause a reference counter unbalance, leading errors such as
> > "target is busy" when trying to unmount overlayfs, even if the
> > filesystem has not active reference.
> >
> > Fix by properly releasing the original file stored in vm_prfile.
> >
> > Fixes: 508fdae3f62dd ("UBUNTU: SAUCE: overlayfs: fix incorrect mnt_id of files opened from map_files")
> > Signed-off-by: Andrea Righi <andrea.righi at canonical.com>
> > ---
> >  fs/overlayfs/file.c | 59 +++++++++++++++++++++++++++------------------
> >  1 file changed, 36 insertions(+), 23 deletions(-)
> >
> > diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> > index 366a4267d5f8..565010d2c9dd 100644
> > --- a/fs/overlayfs/file.c
> > +++ b/fs/overlayfs/file.c
> > @@ -373,17 +373,48 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> >         return ret;
> >  }
> >
> > -/* handle vma->vm_prfile */
> > +/*
> > + * In map_files_get_link() (fs/proc/base.c)
> > + * we need to determine correct path from overlayfs.
> > + * But real_mount(realfile->f_path.mnt) may be not
> > + * equal to real_mount(file->f_path.mnt). In such case
> > + * fdinfo of the same file which was opened from
> > + * /proc/<pid>/map_files/... and "usual" path
> > + * will show different mnt_id.
> > + *
> > + * We solve issue like in aufs by using additional
> > + * field on struct vm_area_struct called "vm_prfile"
> > + * which is used only for fdinfo/"printing" needs.
> > + *
> > + * See also mm/prfile.c
> > + */
> > +#ifdef CONFIG_MMU
> >  static void ovl_vm_prfile_set(struct vm_area_struct *vma,
> >                               struct file *file)
> >  {
> >         get_file(file);
> > -       vma->vm_prfile = file;
> > -#ifndef CONFIG_MMU
> > +       swap(vma->vm_prfile, file);
> > +       /* Drop reference count from previous file value */
> > +       if (file)
> > +               fput(file);
> > +}
> > +#else
> > +static void ovl_vm_prfile_set(struct vm_area_struct *vma,
> > +                             struct file *file)
> > +{
> > +       struct file *vm_region_file = file;
> > +
> >         get_file(file);
> > -       vma->vm_region->vm_prfile = file;
> > -#endif
> > +       get_file(vm_region_file);
> > +       swap(vma->vm_prfile, file);
> > +       swap(vma->vm_region->vm_prfile, vm_region_file);
> > +       /* Drop reference count from previous file values */
> > +       if (file)
> > +               fput(file);
> > +       if (vm_region_file)
> > +               fput(vm_region_file);
> >  }
> > +#endif
> >
> >  static int ovl_mmap(struct file *file, struct vm_area_struct *vma)
> >  {
> > @@ -411,25 +442,7 @@ static int ovl_mmap(struct file *file, struct vm_area_struct *vma)
> >                 vma->vm_file = file;
> >                 fput(realfile);
> >         } else {
> > -               /*
> > -                * In map_files_get_link() (fs/proc/base.c)
> > -                * we need to determine correct path from overlayfs.
> > -                * But real_mount(realfile->f_path.mnt) may be not
> > -                * equal to real_mount(file->f_path.mnt). In such case
> > -                * fdinfo of the same file which was opened from
> > -                * /proc/<pid>/map_files/... and "usual" path
> > -                * will show different mnt_id.
> > -                *
> > -                * We solve issue like in aufs by using additional
> > -                * field on struct vm_area_struct called "vm_prfile"
> > -                * which is used only for fdinfo/"printing" needs.
> > -                *
> > -                * See also mm/prfile.c
> > -                */
> >                 ovl_vm_prfile_set(vma, file);
> > -
> > -               /* Drop reference count from previous vm_file value */
> > -               fput(file);
> >         }
> >
> >         ovl_file_accessed(file);
> > --
> > 2.40.1
> >
> >
> > --
> 
> Dear Andrea,
> 
> I'm almost sure that Focal is not affected by this issue. This issue
> appeared after rebasing to a newer kernel version.
> Have you checked that Focal is affected?

Hi Alex,

I've just checked and focal doesn't seem to be affected (at least the
PoC doesn't trigger the problem).

However, looking at what we're doing here:

static void ovl_vm_prfile_set(struct vm_area_struct *vma,
                              struct file *file)
{
        get_file(file);
        vma->vm_prfile = file;
...

We just blindly overwrite vma->vm_prfile without releasing what could
have been potentially stored there, and maybe that's totally safe in
focal, but I'm not 100% sure that it's always safe.

I'd rather do this:

        get_file(file);
        swap(vma->vm_prfile, file);
        /* Drop reference count from previous file value */
        if (file)
                fput(file);

And in this way the code is also consistent with the other kernels.

What do you think?

-Andrea