Re: [PATCH v2 02/15] riscv: switch arch page-table users to raw PTE helpers

[sashiko-bot@kernel.org]

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

New issues:
- [High] Svnapot huge page set and clear paths have mismatched page_table_check PFN tracking and use unaligned addresses, leading to immediate BUG_ON panics.
- [Medium] The conversion to raw PTE helpers incorrectly bypasses the page_table_check framework for non-NAPOT hugetlb pages.

Pre-existing issues:
- [Critical] KVM stage-2 page table pages are freed before the stage-2 TLB is flushed, causing a use-after-free race.
- [High] huge_pte_clear() and huge_ptep_get_and_clear() fail to clear all contiguous PTEs for non-present entries like migration or hwpoison entries.
--

--- Patch [2]: [PATCH v2 02/15] riscv: switch arch page-table users to raw PTE helpers ---
commit 3f0c1de9839b5c9daaa76b96222e6c57e5096eda
Author: Yunhui Cui <cuiyunhui@bytedance.com>
Subject: riscv: switch arch page-table users to raw PTE helpers

This patch converts RISC-V internal page-table walkers and mutators to
use raw PTE helpers. It also adds a raw get-and-clear helper and updates
the hugetlb Svnapot paths to operate on contiguous blocks using the raw
helper model.

> diff --git a/arch/riscv/kvm/gstage.c b/arch/riscv/kvm/gstage.c
> index c4c3b79567f10..67b2c75f5cec1 100644
> --- a/arch/riscv/kvm/gstage.c
> +++ b/arch/riscv/kvm/gstage.c

[ ... ]

> @@ -371,18 +371,18 @@ bool kvm_riscv_gstage_op_pte(struct kvm_gstage *gstage, gpa_t addr,
>  
>  	WARN_ON(addr & (page_size - 1));
>  
> -	if (!pte_val(ptep_get(ptep)))
> +	if (!pte_val(__ptep_get(ptep)))
>  		return false;
>  
>  	if (ptep_level && !gstage_pte_leaf(ptep)) {
> -		next_ptep = (pte_t *)gstage_pte_page_vaddr(ptep_get(ptep));
> +		next_ptep = (pte_t *)gstage_pte_page_vaddr(__ptep_get(ptep));
>  		next_ptep_level = ptep_level - 1;
>  		ret = gstage_level_to_page_size(gstage, next_ptep_level, &next_page_size);
>  		if (ret)
>  			return false;
>  
>  		if (op == GSTAGE_OP_CLEAR)
> -			set_pte(ptep, __pte(0));
> +			__set_pte(ptep, __pte(0));
>  		for (i = 0; i < PTRS_PER_PTE; i++)
>  			flush |= kvm_riscv_gstage_op_pte(gstage, addr + i * next_page_size,
>  							 &next_ptep[i], next_ptep_level, op);
>  		if (op == GSTAGE_OP_CLEAR)
>  			put_page(virt_to_page(next_ptep));

[Severity: Critical]
This is a pre-existing issue, but does this unconditionally free the child 
page table page before the stage-2 TLB is invalidated? 

The page is freed here by calling put_page() before returning the flush 
boolean to the caller. This allows a concurrent hardware page table walk 
to use the cached TLB directory entry to read from the freed page.

> diff --git a/arch/riscv/mm/hugetlbpage.c b/arch/riscv/mm/hugetlbpage.c
> index a6d217112cf46..5ededac02161d 100644
> --- a/arch/riscv/mm/hugetlbpage.c
> +++ b/arch/riscv/mm/hugetlbpage.c

[ ... ]

> @@ -145,28 +145,52 @@ unsigned long hugetlb_mask_last_page(struct hstate *h)
>  	return 0UL;
>  }
>  
> +static unsigned long napot_hugetlb_block_addr(pte_t pte, unsigned long addr)
> +{
> +	unsigned long order;
> +
> +	if (!pte_napot(pte))
> +		return addr;
> +
> +	order = napot_cont_order(pte);
> +
> +	return addr & napot_cont_mask(order);
> +}
> +
> +static pte_t napot_hugetlb_get_and_clear(struct mm_struct *mm,
> +					 unsigned long addr, pte_t *ptep)
> +{
> +	pte_t pte;
> +
> +	pte = __ptep_get_and_clear_noptc(ptep);
> +	page_table_check_pte_clear(mm, addr, pte_mknonnapot(pte, addr));
> +
> +	return pte;
> +}

[Severity: High]
Does this cause a refcount underflow and trigger a BUG_ON in the 
page_table_check logic? 

When a SVNAPOT huge page is unmapped, get_clear_contig() iteratively calls 
napot_hugetlb_get_and_clear() 16 times for a 64KB block. Each iteration calls 
page_table_check_pte_clear() with the exact same encoded PTE (base_PFN + 8). 
Since set_huge_napot_ptes() tracks the block consecutively, untracking the 
same base PFN + 8 sixteen times will underflow the counter and panic.

[ ... ]

> @@ -207,14 +234,31 @@ static void clear_flush(struct mm_struct *mm,
>  			unsigned long ncontig)
>  {
>  	struct vm_area_struct vma = TLB_FLUSH_VMA(mm, 0);
> +	pte_t pte = __ptep_get(ptep);
>  	unsigned long i, saddr = addr;
>  
> +	addr = napot_hugetlb_block_addr(pte, addr);
> +	if (pte_napot(pte))
> +		ptep = huge_pte_offset(mm, addr,
> +				       napot_cont_size(napot_cont_order(pte)));
> +	saddr = addr;
> +
>  	for (i = 0; i < ncontig; i++, addr += pgsize, ptep++)
> -		ptep_get_and_clear(mm, addr, ptep);
> +		napot_hugetlb_get_and_clear(mm, addr, ptep);
>  
>  	flush_tlb_range(&vma, saddr, addr);
>  }
>  
> +static void set_huge_napot_ptes(struct mm_struct *mm, unsigned long addr,
> +				pte_t *ptep, pte_t pte, unsigned long pte_num)
> +{
> +	unsigned long i;
> +
> +	page_table_check_ptes_set(mm, addr, ptep, pte, pte_num);
> +	for (i = 0; i < pte_num; i++)
> +		__set_pte_at(mm, ptep + i, pte);
> +}

[Severity: High]
Can this pass an unaligned faulting address to page_table_check_ptes_set()
and track the wrong PFNs?

When called from huge_ptep_set_access_flags(), addr is the unaligned 
faulting address, whereas the clear path block-aligns it. This tracks and 
untracks different virtual address ranges. 

Additionally, passing pte_num to page_table_check_ptes_set() tracks 
consecutive PFNs starting from the raw encoded PFN (base_PFN + 8 for order 4) 
instead of the true base PFN.

[ ... ]

> @@ -256,19 +300,18 @@ void set_huge_pte_at(struct mm_struct *mm,
>  
>  	if (!pte_present(pte)) {
>  		for (i = 0; i < pte_num; i++, ptep++, addr += pgsize)
> -			set_ptes(mm, addr, ptep, pte, 1);
> +			__set_ptes(mm, addr, ptep, pte, 1);
>  		return;
>  	}
>  
>  	if (!pte_napot(pte)) {
> -		set_ptes(mm, addr, ptep, pte, 1);
> +		__set_ptes(mm, addr, ptep, pte, 1);
>  		return;
>  	}

[Severity: Medium]
Does this bypass the page_table_check framework for non-NAPOT 2MB huge pages?

The conversion to the raw __set_ptes() helper removes the internal 
page_table_check_ptes_set() call that set_ptes() previously provided. Without 
a manual page_table_check call on this fallback path, memory corruption 
detection is lost for 2MB PMD-mapped huge pages.

[ ... ]

> @@ -306,14 +348,13 @@ pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
>  			      unsigned long addr,
>  			      pte_t *ptep, unsigned long sz)
>  {
> -	size_t pgsize;
> -	pte_t orig_pte = ptep_get(ptep);
> +	pte_t orig_pte = __ptep_get(ptep);
>  	int pte_num;
>  
>  	if (!pte_napot(orig_pte))
> -		return ptep_get_and_clear(mm, addr, ptep);
> +		return __ptep_get_and_clear(mm, addr, ptep);

[Severity: High]
This is a pre-existing issue, but does this fail to clear all contiguous PTEs 
for non-present entries like migration or hwpoison entries?

Since non-present entries do not have the hardware _PAGE_NAPOT bit set, 
pte_napot() evaluates to false. This fallback path returns early after 
clearing only a single 4KB PTE, leaving the remaining 15 stale entries 
in the page table.

[Severity: Medium]
Also related to the conversion to raw PTE helpers, does replacing 
ptep_get_and_clear() with __ptep_get_and_clear() bypass the page_table_check 
framework for non-NAPOT 2MB huge pages?

[ ... ]

> @@ -363,19 +403,16 @@ void huge_pte_clear(struct mm_struct *mm,
>  		    pte_t *ptep,
>  		    unsigned long sz)
>  {
> -	size_t pgsize;
> -	pte_t pte = ptep_get(ptep);
> -	int i, pte_num;
> +	pte_t pte = __ptep_get(ptep);
> +	int pte_num;
>  
>  	if (!pte_napot(pte)) {
> -		pte_clear(mm, addr, ptep);
> +		__pte_clear(mm, addr, ptep);
>  		return;
>  	}

[Severity: High]
This is a pre-existing issue, but does this also fail to clear all contiguous 
PTEs for non-present entries (migration or hwpoison) because pte_napot() 
evaluates to false?

[Severity: Medium]
Similar to the issue above, does replacing pte_clear() with __pte_clear() 
without a manual page_table_check call disable memory corruption tracking 
for non-NAPOT 2MB huge pages?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784201104.git.cuiyunhui@bytedance.com?part=2