[SRU][F][PATCH 1/1] UBUNTU: SAUCE: overlayfs: fix reference count mismatch
Andrea Righi
andrea.righi at canonical.com
Thu Jun 22 12:56:42 UTC 2023
On Thu, Jun 22, 2023 at 02:47:03PM +0200, Aleksandr Mikhalitsyn wrote:
> 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.
I'm totally happy to wait, better to be 100% sure when we touch these
overlayfs SAUCE patches.
Side note: I've started to collect all the little test cases that are
scattered here and there across multiple git commits, bugs, etc. to see
if we can put together a proper test plan for all the custom changes
that we have applied to overlayfs (and shiftfs as well).
I don't think we have anything at the moment that is checking if this
stuff is working as expected for our needs, and that's quite scary,
because, as you mentioned, this stuff is affecting a lot of users.
Do you have any specific test case already that I could possibly
integrate in this potential "overlayfs" test suite?
Thanks,
-Andrea
More information about the kernel-team
mailing list