Re: [PATCH 08/15] x86: Physical address comparisons in fill_p*d/pte

[Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>]

On 2025-02-05 at 16:57:15 -0800, Dave Hansen wrote:
>On 2/4/25 09:33, Maciej Wieczor-Retman wrote:
>> @@ -287,7 +287,7 @@ static pte_t *fill_pte(pmd_t *pmd, unsigned long vaddr)
>>  	if (pmd_none(*pmd)) {
>>  		pte_t *pte = (pte_t *) spp_getpage();
>>  		pmd_populate_kernel(&init_mm, pmd, pte);
>> -		if (pte != pte_offset_kernel(pmd, 0))
>> +		if (__pa(pte) != __pa(pte_offset_kernel(pmd, 0)))
>>  			printk(KERN_ERR "PAGETABLE BUG #03!\n");
>>  	}
>>  	return pte_offset_kernel(pmd, vaddr);
>
>Maciej, could you do a quick check on this and make sure that it doesn't
>hurt code generation on current kernels?
>
>pte_offset_kernel() has an internal __va() so this ends up logically
>being something like:
>
>-	if (     pte  !=      __va(pmd))
>+	if (__pa(pte) != __pa(__va(pmd)))
>
>The __pa() and __va() obviously logically cancel each other out in the
>new version. But if the compiler for whatever reason can't figure this
>out we might end up with worse code.

I browsed through assembly and indeed the __pa(__va()) is longer compared to
only __va() or kasan_reset_tag(__va()).

How about we just open code the *_offset()? What do you think about the patch
below? We can lose the calls to *_index() because they are all zero so we're
only left with insides of the internal __va(). It didn't report any issues in
QEMU at least. The p4d_offset() isn't very pretty here but I think I can make it
better if you like the idea.

----------------------------------------

x86: Physical address comparisons in fill_p*d/pte

Calculating page offset returns a pointer without a tag. When comparing
the calculated offset to a tagged page pointer an error is raised
because they are not equal.

Change pointer comparisons to physical address comparisons as to avoid
issues in KASAN that pointer arithmetic would create. Open code parts
of p*d_offset() to avoid the internal __va() which complicates output
assembly.

Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
---
 arch/x86/mm/init_64.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index ff253648706f..89a86ac34d95 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -251,7 +251,10 @@ static p4d_t *fill_p4d(pgd_t *pgd, unsigned long vaddr)
 	if (pgd_none(*pgd)) {
 		p4d_t *p4d = (p4d_t *)spp_getpage();
 		pgd_populate(&init_mm, pgd, p4d);
-		if (p4d != p4d_offset(pgd, 0))
+
+		if (__pa(p4d) != (pgtable_l5_enabled() ?
+				  __pa(pgd) :
+				  (unsigned long)pgd_val(*pgd) & PTE_PFN_MASK))
 			printk(KERN_ERR "PAGETABLE BUG #00! %p <-> %p\n",
 			       p4d, p4d_offset(pgd, 0));
 	}
@@ -263,7 +266,7 @@ static pud_t *fill_pud(p4d_t *p4d, unsigned long vaddr)
 	if (p4d_none(*p4d)) {
 		pud_t *pud = (pud_t *)spp_getpage();
 		p4d_populate(&init_mm, p4d, pud);
-		if (pud != pud_offset(p4d, 0))
+		if (__pa(pud) != (p4d_val(*p4d) & p4d_pfn_mask(*p4d)))
 			printk(KERN_ERR "PAGETABLE BUG #01! %p <-> %p\n",
 			       pud, pud_offset(p4d, 0));
 	}
@@ -275,7 +278,7 @@ static pmd_t *fill_pmd(pud_t *pud, unsigned long vaddr)
 	if (pud_none(*pud)) {
 		pmd_t *pmd = (pmd_t *) spp_getpage();
 		pud_populate(&init_mm, pud, pmd);
-		if (pmd != pmd_offset(pud, 0))
+		if (__pa(pmd) != (pud_val(*pud) & pud_pfn_mask(*pud)))
 			printk(KERN_ERR "PAGETABLE BUG #02! %p <-> %p\n",
 			       pmd, pmd_offset(pud, 0));
 	}
@@ -287,7 +290,7 @@ static pte_t *fill_pte(pmd_t *pmd, unsigned long vaddr)
 	if (pmd_none(*pmd)) {
 		pte_t *pte = (pte_t *) spp_getpage();
 		pmd_populate_kernel(&init_mm, pmd, pte);
-		if (pte != pte_offset_kernel(pmd, 0))
+		if (__pa(pte) != (pmd_val(*pmd) & pmd_pfn_mask(*pmd)))
 			printk(KERN_ERR "PAGETABLE BUG #03!\n");
 	}
 	return pte_offset_kernel(pmd, vaddr);


>
>If it generates crummy code we might want to do this differently like
>avoiding pte_offset_kernel() and adding some other helper that's more
>direct and to the point.

-- 
Kind regards
Maciej Wieczór-Retman