mm/huge_memory: Fix use of NULL folio in move_pages_huge_pmd()
Subsystems (3)
PATCH 1/2 mm/huge_memory: Fix use of NULL folio in move_pages_huge_pmd()
move_pages_huge_pmd() handles UFFDIO_MOVE for both normal THPs and huge zero pages. For the huge zero page path, src_folio is explicitly set to NULL (used as a sentinel to skip folio operations like lock and rmap).
In the huge zero page branch, src_folio is NULL, so folio_mk_pmd(NULL, pgprot) passes NULL through folio_pfn() and page_to_pfn(). With SPARSEMEM_VMEMMAP this silently produces a bogus PFN, installing a PMD pointing to non-existent physical memory. On other memory models it is a NULL dereference.
Use page_folio(src_page) to obtain the valid huge zero folio from the page, which was obtained from pmd_page() and remains valid throughout.
--- mm/huge_memory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
mm/huge_memory.c +1 -1
@@ -2794,7 +2794,7 @@ int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pm _dst_pmd =pmd_mkwrite (pmd_mkdirty (_dst_pmd), dst_vma); }else { src_pmdval =pmdp_huge_clear_flush (src_vma, src_addr, src_pmd); - _dst_pmd =folio_mk_pmd (src_folio, dst_vma->vm_page_prot); + _dst_pmd =folio_mk_pmd (page_folio (src_page), dst_vma->vm_page_prot); }set_pmd_at (mm, dst_addr, dst_pmd, _dst_pmd);
Signature · 1 line
2.51.2
David Hildenbrand (Arm)
maintainerQuoted 25 lines · Chris Down
move_pages_huge_pmd()handlesUFFDIO_MOVEfor both normal THPs and huge zero pages. For the huge zero page path,src_foliois explicitly set to NULL (used as a sentinel to skip folio operations like lock and rmap).In the huge zero page branch,
src_foliois NULL, sofolio_mk_pmd(NULL, pgprot)passes NULL throughfolio_pfn()andpage_to_pfn(). WithSPARSEMEM_VMEMMAPthis silently produces a bogus PFN, installing a PMD pointing to non-existent physical memory. On other memory models it is a NULL dereference.Use
page_folio(src_page)to obtain the valid huge zero folio from the page, which was obtained frompmd_page()and remains valid throughout.Fixes e3981db444a0 ("mm: add folio_mk_pmd()")Signed-off-by Chris Down <[email protected]>--- mm/huge_memory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)mm/huge_memory.c +0
-0
mm/huge_memory.c int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pm · line 2800
Quoted 8 lines
mm/huge_memory.c +1
-1@@ -2794,7 +2794,7 @@ int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pm _dst_pmd =pmd_mkwrite (pmd_mkdirty (_dst_pmd), dst_vma); }else { src_pmdval =pmdp_huge_clear_flush (src_vma, src_addr, src_pmd); - _dst_pmd =folio_mk_pmd (src_folio, dst_vma->vm_page_prot); + _dst_pmd =folio_mk_pmd (page_folio (src_page), dst_vma->vm_page_prot); }set_pmd_at (mm, dst_addr, dst_pmd, _dst_pmd);
God this code is horrible.
Why can't we simply move the PMD like move_huge_pmd() would do and make this code less error prone?
_dst_pmd = src_pmdval;
According to validate_move_areas(), the permissions of both VMAs are equal.
We might have to handle it just like move_huge_pmd():
_dst_pmd = move_soft_dirty_pmd(src_pmdval);
To also set the softdirty bit.
Then, we would want to just clear uffd.
_dst_pmd = move_soft_dirty_pmd(src_pmdval); _dst_pmd = clear_uffd_wp_pmd(_dst_pmd);
Signature · 3 lines
Cheers, David
David Hildenbrand (Arm)
maintainerQuoted 49 lines · David Hildenbrand (Arm)
Quoted 31 lines · Chris Down
move_pages_huge_pmd()handlesUFFDIO_MOVEfor both normal THPs and huge zero pages. For the huge zero page path,src_foliois explicitly set to NULL (used as a sentinel to skip folio operations like lock and rmap).In the huge zero page branch,
src_foliois NULL, sofolio_mk_pmd(NULL, pgprot)passes NULL throughfolio_pfn()andpage_to_pfn(). WithSPARSEMEM_VMEMMAPthis silently produces a bogus PFN, installing a PMD pointing to non-existent physical memory. On other memory models it is a NULL dereference.Use
page_folio(src_page)to obtain the valid huge zero folio from the page, which was obtained frompmd_page()and remains valid throughout.Fixes e3981db444a0 ("mm: add folio_mk_pmd()")Signed-off-by Chris Down <[email protected]>---
mm/huge_memory.c| 2 +-1 file changed, 1 insertion(+), 1 deletion(-)mm/huge_memory.c +0
-0@@ -2794,7 +2794,7 @@ int move_pages_huge_pmd(struct mm_struct *mm,
pmd_t*dst_pmd,pmd_t*src_pmd, pm
_dst_pmd=pmd_mkwrite(pmd_mkdirty(_dst_pmd), dst_vma); } else {
src_pmdval=pmdp_huge_clear_flush(src_vma, src_addr, src_pmd); -_dst_pmd=folio_mk_pmd(src_folio, dst_vma->vm_page_prot); +_dst_pmd= folio_mk_pmd(page_folio(src_page), dst_vma-Quoted 4 lines
Quoted 1 line
vm_page_prot);}
set_pmd_at(mm, dst_addr, dst_pmd, _dst_pmd);God this code is horrible.
Why can't we simply move the PMD like
move_huge_pmd()would do and make this code less error prone?_dst_pmd =
src_pmdval;According to
validate_move_areas(), the permissions of both VMAs are equal.We might have to handle it just like
move_huge_pmd():
Quoted 9 lines
_dst_pmd =
move_soft_dirty_pmd(src_pmdval);To also set the softdirty bit.
Then, we would want to just clear uffd.
_dst_pmd =
move_soft_dirty_pmd(src_pmdval);
_dst_pmd =clear_uffd_wp_pmd(_dst_pmd);
Chris, do you have time to follow up, or should I look into it? The issue looks quite bad, so we should tackle it ASAP.
Signature · 3 lines
Cheers, David
Quoted 2 lines · David Hildenbrand (Arm)
Chris, do you have time to follow up, or should I look into it? The issue looks quite bad, so we should tackle it ASAP.
Thanks for the poke, I will send v2 today.