[SRU][F][PATCH 1/1] UBUNTU: SAUCE: overlayfs: fix reference count mismatch
Aleksandr Mikhalitsyn
aleksandr.mikhalitsyn at canonical.com
Wed Jun 28 15:01:29 UTC 2023
On Thu, Jun 22, 2023 at 2:56 PM Andrea Righi <andrea.righi at canonical.com> wrote:
>
> 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.
Dear Andrea,
>
> I'm totally happy to wait, better to be 100% sure when we touch these
> overlayfs SAUCE patches.
I've added a comment with the result of my investigation of this issue.
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2016398/comments/17
>
> 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?
It's a very good idea!
First of all, this patch contains testcase in the description:
https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/focal/commit/fs/overlayfs/file.c?h=master-next&id=28eab192cf0e37156fc41b36f06790d5ca984834
Another test that comes to my mind is to mount overlayfs on top of
shiftfs and do some basic files/directories operations.
Kind regards,
Alex
>
> Thanks,
> -Andrea
More information about the kernel-team
mailing list