[SRU][L/K/J][PATCH 1/1] UBUNTU: SAUCE: overlayfs: fix reference count mismatch

[Andrea Righi]

On Thu, Jun 22, 2023 at 12:08:19PM +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 | 25 +++++++++++++++++++++----
> >  1 file changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> > index e3c6834d8d0f1..6230364ae4b41 100644
> > --- a/fs/overlayfs/file.c
> > +++ b/fs/overlayfs/file.c
> > @@ -504,16 +504,33 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> >   *
> >   * 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)
> >  {
> > --
> > 2.40.1
> 
> Dear Andrea,
> 
> Is this supposed to be in the Jammy branch?
> 
> Please take a look on this:
> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/jammy/commit/?id=07648d68cea786d2ff599b51139013044ec59a8a
> 
> Kind regards,
> Alex

Yes, we are not re-introducing this bug, because with this patch we are
doing fput() on the old file, note the `swap(vma->vm_prfile, file)`.

In LP: #1973620 the bug was caused by the fact that we stored the file
in vma->pr_file without keeping a reference on it, basically we were
doing this:

  get_file(file);
  vma->vm_prfile = file;
  fput(file)

Then when the next fput() happened the file was incorrectly released
leading to the use-after-free / NULL pointer dereference.

-Andrea