[bug report] mm/mremap: complete refactor of move_vma()

[bug report] mm/mremap: complete refactor of move_vma()

[Dan Carpenter]

Hello Lorenzo Stoakes,

The patch b714ccb02a76: "mm/mremap: complete refactor of move_vma()"
from Mar 10, 2025, leads to the following static checker warning:

	mm/mremap.c:1920 move_vma()
	error: uninitialized symbol 'new_vma'.

mm/mremap.c
    1895 static unsigned long move_vma(struct vma_remap_struct *vrm)
    1896 {
    1897 	struct mm_struct *mm = current->mm;
    1898 	struct vm_area_struct *new_vma;
    1899 	unsigned long hiwater_vm;
    1900 	int err;
    1901 
    1902 	err = prep_move_vma(vrm);
    1903 	if (err)
    1904 		return err;
    1905 
    1906 	/* If accounted, charge the number of bytes the operation will use. */
    1907 	if (!vrm_charge(vrm))
    1908 		return -ENOMEM;
    1909 
    1910 	/* We don't want racing faults. */
    1911 	vma_start_write(vrm->vma);
    1912 
    1913 	/* Perform copy step. */
    1914 	err = copy_vma_and_data(vrm, &new_vma);
    1915 	/*
    1916 	 * If we established the copied-to VMA, we attempt to recover from the
    1917 	 * error by setting the destination VMA to the source VMA and unmapping
    1918 	 * it below.
    1919 	 */
--> 1920 	if (err && !new_vma)
                            ^^^^^^^
new_vma isn't set on the first error path in copy_vma_and_data().

    1921 		return err;
    1922 
    1923 	/*
    1924 	 * If we failed to move page tables we still do total_vm increment
    1925 	 * since do_munmap() will decrement it by old_len == new_len.
    1926 	 *
    1927 	 * Since total_vm is about to be raised artificially high for a

regards,
dan carpenter

Re: [bug report] mm/mremap: complete refactor of move_vma()

[Lorenzo Stoakes]

On Wed, Jun 25, 2025 at 10:22:49AM -0500, Dan Carpenter wrote:
> Hello Lorenzo Stoakes,
>
> The patch b714ccb02a76: "mm/mremap: complete refactor of move_vma()"
> from Mar 10, 2025, leads to the following static checker warning:
>
> 	mm/mremap.c:1920 move_vma()
> 	error: uninitialized symbol 'new_vma'.
>
> mm/mremap.c
>     1895 static unsigned long move_vma(struct vma_remap_struct *vrm)
>     1896 {
>     1897 	struct mm_struct *mm = current->mm;
>     1898 	struct vm_area_struct *new_vma;
>     1899 	unsigned long hiwater_vm;
>     1900 	int err;
>     1901
>     1902 	err = prep_move_vma(vrm);
>     1903 	if (err)
>     1904 		return err;
>     1905
>     1906 	/* If accounted, charge the number of bytes the operation will use. */
>     1907 	if (!vrm_charge(vrm))
>     1908 		return -ENOMEM;
>     1909
>     1910 	/* We don't want racing faults. */
>     1911 	vma_start_write(vrm->vma);
>     1912
>     1913 	/* Perform copy step. */
>     1914 	err = copy_vma_and_data(vrm, &new_vma);
>     1915 	/*
>     1916 	 * If we established the copied-to VMA, we attempt to recover from the
>     1917 	 * error by setting the destination VMA to the source VMA and unmapping
>     1918 	 * it below.
>     1919 	 */
> --> 1920 	if (err && !new_vma)
>                             ^^^^^^^
> new_vma isn't set on the first error path in copy_vma_and_data().

Hmm, the first error path is:

	if (!new_vma) {
		vrm_uncharge(vrm);
		*new_vma_ptr = NULL;
		return -ENOMEM;
	}

Which explicitly sets new_vma = NULL (new_vma_ptr = &new_vma)

The final return there is:

	*new_vma_ptr = new_vma;
	return err;

Which also sets it.

So I'm not sure what's wrong here? Am I missing something?

>
>     1921 		return err;
>     1922
>     1923 	/*
>     1924 	 * If we failed to move page tables we still do total_vm increment
>     1925 	 * since do_munmap() will decrement it by old_len == new_len.
>     1926 	 *
>     1927 	 * Since total_vm is about to be raised artificially high for a
>
> regards,
> dan carpenter
>

Re: [bug report] mm/mremap: complete refactor of move_vma()

[Dan Carpenter]

On Wed, Jun 25, 2025 at 04:28:51PM +0100, Lorenzo Stoakes wrote:
> On Wed, Jun 25, 2025 at 10:22:49AM -0500, Dan Carpenter wrote:
> > Hello Lorenzo Stoakes,
> >
> > The patch b714ccb02a76: "mm/mremap: complete refactor of move_vma()"
> > from Mar 10, 2025, leads to the following static checker warning:
> >
> > 	mm/mremap.c:1920 move_vma()
> > 	error: uninitialized symbol 'new_vma'.
> >
> > mm/mremap.c
> >     1895 static unsigned long move_vma(struct vma_remap_struct *vrm)
> >     1896 {
> >     1897 	struct mm_struct *mm = current->mm;
> >     1898 	struct vm_area_struct *new_vma;
> >     1899 	unsigned long hiwater_vm;
> >     1900 	int err;
> >     1901
> >     1902 	err = prep_move_vma(vrm);
> >     1903 	if (err)
> >     1904 		return err;
> >     1905
> >     1906 	/* If accounted, charge the number of bytes the operation will use. */
> >     1907 	if (!vrm_charge(vrm))
> >     1908 		return -ENOMEM;
> >     1909
> >     1910 	/* We don't want racing faults. */
> >     1911 	vma_start_write(vrm->vma);
> >     1912
> >     1913 	/* Perform copy step. */
> >     1914 	err = copy_vma_and_data(vrm, &new_vma);
> >     1915 	/*
> >     1916 	 * If we established the copied-to VMA, we attempt to recover from the
> >     1917 	 * error by setting the destination VMA to the source VMA and unmapping
> >     1918 	 * it below.
> >     1919 	 */
> > --> 1920 	if (err && !new_vma)
> >                             ^^^^^^^
> > new_vma isn't set on the first error path in copy_vma_and_data().
> 
> Hmm, the first error path is:
> 
> 	if (!new_vma) {
> 		vrm_uncharge(vrm);
> 		*new_vma_ptr = NULL;
> 		return -ENOMEM;
> 	}
> 
> Which explicitly sets new_vma = NULL (new_vma_ptr = &new_vma)
> 
> The final return there is:
> 
> 	*new_vma_ptr = new_vma;
> 	return err;
> 
> Which also sets it.
> 
> So I'm not sure what's wrong here? Am I missing something?
> 

There is an earlier error path.

  1750  static int copy_vma_and_data(struct vma_remap_struct *vrm,
  1751                               struct vm_area_struct **new_vma_ptr)
  1752  {
  1753          unsigned long internal_offset = vrm->addr - vrm->vma->vm_start;
  1754          unsigned long internal_pgoff = internal_offset >> PAGE_SHIFT;
  1755          unsigned long new_pgoff = vrm->vma->vm_pgoff + internal_pgoff;
  1756          unsigned long moved_len;
  1757          struct vm_area_struct *vma = vrm->vma;
  1758          struct vm_area_struct *new_vma;
  1759          int err = 0;
  1760          PAGETABLE_MOVE(pmc, NULL, NULL, vrm->addr, vrm->new_addr, vrm->old_len);
  1761          bool relocate_anon = should_relocate_anon(vrm, &pmc, &err);
  1762  
  1763          if (err)
  1764                  return err;
                        ^^^^^^^^^^^
Here.

  1765  
  1766  again:
  1767          new_vma = copy_vma(&vma, vrm->new_addr, vrm->new_len, new_pgoff,
  1768                             &pmc.need_rmap_locks, &relocate_anon);
  1769          if (!new_vma) {
  1770                  vrm_uncharge(vrm);
  1771                  *new_vma_ptr = NULL;
  1772                  return -ENOMEM;
  1773          }

regards,
dan carpenter

Re: [bug report] mm/mremap: complete refactor of move_vma()

[Lorenzo Stoakes]

On Wed, Jun 25, 2025 at 06:35:59PM +0300, Dan Carpenter wrote:
> On Wed, Jun 25, 2025 at 04:28:51PM +0100, Lorenzo Stoakes wrote:
> There is an earlier error path.
>
>   1750  static int copy_vma_and_data(struct vma_remap_struct *vrm,
>   1751                               struct vm_area_struct **new_vma_ptr)
>   1752  {
>   1753          unsigned long internal_offset = vrm->addr - vrm->vma->vm_start;
>   1754          unsigned long internal_pgoff = internal_offset >> PAGE_SHIFT;
>   1755          unsigned long new_pgoff = vrm->vma->vm_pgoff + internal_pgoff;
>   1756          unsigned long moved_len;
>   1757          struct vm_area_struct *vma = vrm->vma;
>   1758          struct vm_area_struct *new_vma;
>   1759          int err = 0;
>   1760          PAGETABLE_MOVE(pmc, NULL, NULL, vrm->addr, vrm->new_addr, vrm->old_len);
>   1761          bool relocate_anon = should_relocate_anon(vrm, &pmc, &err);
>   1762
>   1763          if (err)
>   1764                  return err;
>                         ^^^^^^^^^^^
> Here.

The bug report is against the wrong patch, the error was introduced by
'mm/mremap: add MREMAP_MUST_RELOCATE_ANON' (i.e. this check).

I'm looking at a massive refactor which removess this and checked against
'mm/mremap: complete refactor of move_vma()' which is why I missed this :)

Since I'm respinning anyway I'll just ask Andrew to drop the series for now
until I come back with the reworked version.

Thanks for the report!

>
>   1765
>   1766  again:
>   1767          new_vma = copy_vma(&vma, vrm->new_addr, vrm->new_len, new_pgoff,
>   1768                             &pmc.need_rmap_locks, &relocate_anon);
>   1769          if (!new_vma) {
>   1770                  vrm_uncharge(vrm);
>   1771                  *new_vma_ptr = NULL;
>   1772                  return -ENOMEM;
>   1773          }
>
> regards,
> dan carpenter
>
>