diff mbox series

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

Message ID 20230622093731.632666-2-andrea.righi@canonical.com
State New
Headers show
Series UBUNTU: SAUCE: overlayfs: fix reference count mismatch | expand

Commit Message

Andrea Righi June 22, 2023, 9:37 a.m. UTC 
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@canonical.com>
---
 fs/overlayfs/file.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

Comments

Aleksandr Mikhalitsyn June 22, 2023, 10:08 a.m. UTC | #1 
On Thu, Jun 22, 2023 at 11:38 AM Andrea Righi
<andrea.righi@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@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

>
>
> --
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Andrea Righi June 22, 2023, 12:27 p.m. UTC | #2 
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@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@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
Aleksandr Mikhalitsyn June 28, 2023, 3:36 p.m. UTC | #3 
On Thu, Jun 22, 2023 at 11:38 AM Andrea Righi
<andrea.righi@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@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

Looks good to me.

Thanks,
Alex

>
>  static int ovl_mmap(struct file *file, struct vm_area_struct *vma)
>  {
> --
> 2.40.1
>
>
> --
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Stefan Bader June 30, 2023, 7:40 a.m. UTC | #4 
On 22.06.23 11:37, Andrea Righi 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@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---

Kinetic may or may not be updated. It reaches EOL soon and right now 
there is no planned normal SRU cycle for it.

-Stefan

>   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)
>   {
diff mbox series

Patch

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)
 {