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

[Aleksandr Mikhalitsyn]

On Thu, Jun 22, 2023 at 2:20 PM Andrea Righi <andrea.righi at canonical.com> wrote:
>
> 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).

Thanks!

>
> 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?

It's an interesting point. Seems like this can only be reproduced when
one overlayfs is stacked with another one overlayfs.
Usually, vma->vm_prfile is NULL when we enter the ovl_vm_prfile_set function.

If you don't mind, I'll take a close look at this place on the weekend
and check everything. I know that this place is very-very tricky and
we
have made a few bugs right there before :) Overlayfs is very popular
and I'm afraid that we can affect many users.

The same applies to the Jammy/Kinetic and other trees.

If you wanted to merge this really quick and this can't wait please
tell me. I'll try to take a closer look at that closely today evening
or tomorrow then.

Kind regards,
Alex

>
> -Andrea