Skip to contentSkip to conversation

mm/huge_memory: Fix use of NULL folio in move_pages_huge_pmd()

PATCH Superseded Chris Down 2 participants 4 messages Feb 18 – Feb 26

Subsystems (3)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Chris Down

author
#0 lore raw

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.

Fixes e3981db444a0 ("mm: add folio_mk_pmd()")
---
 mm/huge_memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/huge_memory.c +1 -1
index 44ff8a648afd..fed57951a7cd 100644
@@ -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
#1 lore raw
Quoted 25 lines · Chris Down

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.

Fixes e3981db444a0 ("mm: add folio_mk_pmd()")
---
  mm/huge_memory.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
mm/huge_memory.c +0 -0
index 44ff8a648afd..fed57951a7cd 100644

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
#2 lore raw
Quoted 49 lines · David Hildenbrand (Arm)
Quoted 31 lines · Chris Down

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.

Fixes e3981db444a0 ("mm: add folio_mk_pmd()")

---
  mm/huge_memory.c | 2 +-

  1 file changed, 1 insertion(+), 1 deletion(-)
mm/huge_memory.c +0 -0
index 44ff8a648afd..fed57951a7cd 100644
@@ -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
author
#3 lore raw
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.