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

[Dan Carpenter <dan.carpenter@linaro.org>]

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