Re: [PATCH 4/9] mm: use hw_pte_t for generic PTE table storage

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] copy_present_ptes batches PTEs in shared mappings without FPB_RESPECT_WRITE, granting the child process unintended write access to read-only pages and bypassing filesystem write notification.
- [High] Page walk callbacks wp_pte() and clean_record_pte() access hardware bits on potentially non-present PTEs, risking metadata corruption.
--

--- Patch [4]: [PATCH 4/9] mm: use hw_pte_t for generic PTE table storage ---
commit 9f475c080b8384027672c2a2cfdb3246a47fd906
Author: Muhammad Usama Anjum <[email protected]>
Subject: mm: use hw_pte_t for generic PTE table storage

This commit converts generic page-table interfaces to use hw_pte_t pointers for
table storage instead of pte_t pointers. It retains pte_t for logical PTE
values, changing the vocabulary without altering representation or behavior.

> diff --git a/mm/mapping_dirty_helpers.c b/mm/mapping_dirty_helpers.c
> --- a/mm/mapping_dirty_helpers.c
> +++ b/mm/mapping_dirty_helpers.c
> @@ -31,7 +31,7 @@ struct wp_walk {
>   * The function write-protects a pte and records the range in
>   * virtual address space of touched ptes for efficient range TLB flushes.
>   */
> -static int wp_pte(pte_t *pte, unsigned long addr, unsigned long end,
> +static int wp_pte(hw_pte_t *pte, unsigned long addr, unsigned long end,
>  		  struct mm_walk *walk)
>  {
>  	struct wp_walk *wpwalk = walk->private;
>  	pte_t ptent = ptep_get(pte);
>  
>  	if (pte_write(ptent)) {

[Severity: High]
This isn't a bug introduced by this patch, but does this code modify hardware
bits on non-present PTEs?

The page walk callback wp_pte() processes all PTEs within a mapping range,
including non-present entries like migration or swap entries. It checks
pte_write() directly without verifying pte_present() first.

If a swap or migration entry overlaps with hardware writable bits, this check
might pass, causing pte_wrprotect() to be called. This can silently corrupt
swap or migration metadata, leading to memory corruption when the entry is
later resolved.

[ ... ]

> @@ -86,7 +86,7 @@ struct clean_walk {
>   * in the address_space, as well as the first and last of the bits
>   * touched.
>   */
> -static int clean_record_pte(pte_t *pte, unsigned long addr,
> +static int clean_record_pte(hw_pte_t *pte, unsigned long addr,
>  			    unsigned long end, struct mm_walk *walk)
>  {
>  	struct wp_walk *wpwalk = walk->private;
>  	struct clean_walk *cwalk = to_clean_walk(wpwalk);
>  	pte_t ptent = ptep_get(pte);
>  
>  	if (pte_dirty(ptent)) {

[Severity: High]
This is a pre-existing issue, but can this inadvertently corrupt non-present
PTEs?

Similar to wp_pte() above, clean_record_pte() evaluates pte_dirty() on the
entry without checking if the PTE is actually present. Calling present-PTE
accessors on non-present entries produces undefined results. If this evaluates
to true for a swap or migration entry, calling pte_mkclean() will overwrite
and corrupt the swap metadata.

[ ... ]

> diff --git a/mm/memory.c b/mm/memory.c
> --- a/mm/memory.c
> +++ b/mm/memory.c
[ ... ]
> @@ -1205,7 +1205,7 @@ static __always_inline void __copy_present_ptes(struct vm_area_struct *dst_vma,
>   */
>  static inline int
>  copy_present_ptes(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
> -		 pte_t *dst_pte, pte_t *src_pte, pte_t pte, unsigned long addr,
> +		 hw_pte_t *dst_pte, hw_pte_t *src_pte, pte_t pte, unsigned long addr,
>  		 int max_nr, int *rss, struct folio **prealloc)
>  {
>  	fpb_t flags = FPB_MERGE_WRITE;
>  	struct page *page;
>  	struct folio *folio;
>  	int err, nr;
>  
>  	page = vm_normal_page(src_vma, addr, pte);
>  	if (unlikely(!page))
>  		goto copy_pte;
>  
>  	folio = page_folio(page);
>  
>  	if (unlikely(!*prealloc && folio_test_large(folio) && max_nr != 1)) {
>  		if (!(src_vma->vm_flags & VM_SHARED))
>  			flags |= FPB_RESPECT_DIRTY;
>  		if (vma_soft_dirty_enabled(src_vma))
>  			flags |= FPB_RESPECT_SOFT_DIRTY;
>  
>  		nr = folio_pte_batch_flags(folio, src_vma, src_pte, &pte, max_nr, flags);

[Severity: Critical]
This is a pre-existing issue, but does this code allow read-only PTEs to become
writable in the child process?

In copy_present_ptes(), when batching PTEs for a shared mapping, the flags
variable includes FPB_MERGE_WRITE but the FPB_RESPECT_WRITE flag is missing.
This causes folio_pte_batch_flags() to group PTEs with differing write
permissions.

If a batch contains both writable and read-only PTEs (for example, a 4K page
was made writable via page_mkwrite() while the rest of the THP remains
read-only), FPB_MERGE_WRITE ensures the batched output PTE is marked writable.

For shared mappings, __copy_present_ptes() calls pte_mkclean() but does not
write-protect the PTE. Consequently, set_ptes() installs this clean but
writable PTE across the entire batched range in the child process.

When the child writes to these newly writable pages, the hardware sets the
dirty bit without a page fault. This bypasses the filesystem write
notifications, which can allow writes to unallocated blocks in sparse files
and silently drop data.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4