* [bug report] mm/mremap: complete refactor of move_vma()
@ 2025-06-25 15:22 Dan Carpenter
2025-06-25 15:28 ` Lorenzo Stoakes
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2025-06-25 15:22 UTC (permalink / raw)
To: lorenzo.stoakes; +Cc: linux-mm
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] mm/mremap: complete refactor of move_vma()
2025-06-25 15:22 [bug report] mm/mremap: complete refactor of move_vma() Dan Carpenter
@ 2025-06-25 15:28 ` Lorenzo Stoakes
2025-06-25 15:35 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Lorenzo Stoakes @ 2025-06-25 15:28 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-mm
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] mm/mremap: complete refactor of move_vma()
2025-06-25 15:28 ` Lorenzo Stoakes
@ 2025-06-25 15:35 ` Dan Carpenter
2025-06-25 15:41 ` Lorenzo Stoakes
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2025-06-25 15:35 UTC (permalink / raw)
To: Lorenzo Stoakes; +Cc: linux-mm
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] mm/mremap: complete refactor of move_vma()
2025-06-25 15:35 ` Dan Carpenter
@ 2025-06-25 15:41 ` Lorenzo Stoakes
0 siblings, 0 replies; 4+ messages in thread
From: Lorenzo Stoakes @ 2025-06-25 15:41 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-mm
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
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-06-25 15:41 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-06-25 15:22 [bug report] mm/mremap: complete refactor of move_vma() Dan Carpenter
2025-06-25 15:28 ` Lorenzo Stoakes
2025-06-25 15:35 ` Dan Carpenter
2025-06-25 15:41 ` Lorenzo Stoakes
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox