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

[Aleksandr Mikhalitsyn]

On Thu, Jun 29, 2023 at 8:03 AM Andrea Righi <andrea.righi at canonical.com> wrote:
>
> On Wed, Jun 28, 2023 at 05:32:21PM +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);
> >
> > I'm not sure that we need to touch this line because it is not from
> > the Ubuntu Sauce patch, but from the original Linux kernel tree.
> > Removing it can cause a leak.
>
> That line is not from the SAUCE patch, but before my change
> ovl_vm_prfile_set() wasn't doing an fput(file). Now that
> ovl_vm_prfile_set() is also taking care of doing fput(file), I think we
> need to remove that line, otherwise we would release the file twice.

fput inside ovl_vm_prfile_set releases the struct file that was in
vma->vm_prfile (if it is not NULL),
but this fput(file) is not about that. It releases and an old struct
file reference that was in vma->vm_file
when ovl_mmap() was called. This reference was taken in mmap_region()
just before the call_mmap() call.
So, as in ovl_mmap we replace vma->vm_file with a new one, we need to
release an old reference.

Please note, that vma->vm_file != vma->vm_prfile.

Kind regards,
Alex

>
> -Andrea