[[e-mail]:,Re:,[syzbot,mm?] BUG: Bad page map (7)]

Commit Message

Matthew Wilcox Sept. 11, 2023, 2:57 p.m. UTC

Just to get a few more eyes on this ... you seem like the experts on
inverted PTEs.

syzbot says this works, but it's only going to have done limited
testing.

----- Forwarded message from Matthew Wilcox <[e-mail]> -----

Date: Mon, 11 Sep 2023 14:26:09 +0100
From: Matthew Wilcox <[e-mail]>
To: Yin Fengwei <[e-mail]>
Cc: syzbot <[e-mail]>,
	[e-mail], [e-mail],
	[e-mail], [e-mail]
Subject: Re: [syzbot] [mm?] BUG: Bad page map (7)

On Mon, Sep 11, 2023 at 03:12:27PM +0800, Yin Fengwei wrote:
>  
> +static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
> +               pte_t *ptep, pte_t pte, unsigned int nr)
> +{
> +       bool protnone = (pte_flags(pte) & (_PAGE_PROTNONE | _PAGE_PRESENT))
> +                       == _PAGE_PROTNONE;
> +
> +       page_table_check_ptes_set(mm, ptep, pte, nr);
> +
> +       for(;;) {
> +               native_set_pte(ptep, pte);
> +               if (--nr == 0)
> +                       break;
> +
> +               ptep++;
> +               if (protnone)
> +                       pte = __pte(pte_val(pte) - (1UL << PFN_PTE_SHIFT));
> +               else
> +                       pte = __pte(pte_val(pte) + (1UL << PFN_PTE_SHIFT));
> +       }
> +}
> +#define set_ptes set_ptes

Thanks for figuring this out.  I don't think I would have been able to!

I think this solution probably breaks pgtable-2level configs,
unfortunately.  How about this?  If other architectures decide to adopt
the inverted page table entry in the future, it'll work for them too.

#syz test



----- End forwarded message -----

Comments

David Hildenbrand Sept. 11, 2023, 5:50 p.m. UTC | #1

>   #ifndef set_ptes
>   /**
>    * set_ptes - Map consecutive pages to a contiguous range of addresses.
> @@ -231,7 +235,10 @@ static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
>   		if (--nr == 0)
>   			break;
>   		ptep++;
> -		pte = __pte(pte_val(pte) + (1UL << PFN_PTE_SHIFT));
> +		if (__pte_needs_invert(pte_val(pte)))
> +			pte = __pte(pte_val(pte) - (1UL << PFN_PTE_SHIFT));
> +		else
> +			pte = __pte(pte_val(pte) + (1UL << PFN_PTE_SHIFT));
>   	}

Maybe we want some pte_advance() [or similar, you get the spirit] instead?

Leaking this inverted-pte logic into common code really does look nasty.

Thomas Gleixner Sept. 13, 2023, 9:28 p.m. UTC | #2

On Mon, Sep 11 2023 at 19:50, David Hildenbrand wrote:

>>   #ifndef set_ptes
>>   /**
>>    * set_ptes - Map consecutive pages to a contiguous range of addresses.
>> @@ -231,7 +235,10 @@ static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
>>   		if (--nr == 0)
>>   			break;
>>   		ptep++;
>> -		pte = __pte(pte_val(pte) + (1UL << PFN_PTE_SHIFT));
>> +		if (__pte_needs_invert(pte_val(pte)))
>> +			pte = __pte(pte_val(pte) - (1UL << PFN_PTE_SHIFT));
>> +		else
>> +			pte = __pte(pte_val(pte) + (1UL << PFN_PTE_SHIFT));
>>   	}
>
> Maybe we want some pte_advance() [or similar, you get the spirit] instead?
>
> Leaking this inverted-pte logic into common code really does look nasty.

Yes please

Patch

diff --git a/arch/x86/include/asm/pgtable-2level.h b/arch/x86/include/asm/pgtable-2level.h
index e9482a11ac52..a89be3e9b032 100644
--- a/arch/x86/include/asm/pgtable-2level.h
+++ b/arch/x86/include/asm/pgtable-2level.h
@@ -123,9 +123,6 @@  static inline u64 flip_protnone_guard(u64 oldval, u64 val, u64 mask)
 	return val;
 }
 
-static inline bool __pte_needs_invert(u64 val)
-{
-	return false;
-}
+#define __pte_needs_invert(val)	false
 
 #endif /* _ASM_X86_PGTABLE_2LEVEL_H */
diff --git a/arch/x86/include/asm/pgtable-invert.h b/arch/x86/include/asm/pgtable-invert.h
index a0c1525f1b6f..f21726add655 100644
--- a/arch/x86/include/asm/pgtable-invert.h
+++ b/arch/x86/include/asm/pgtable-invert.h
@@ -17,6 +17,7 @@  static inline bool __pte_needs_invert(u64 val)
 {
 	return val && !(val & _PAGE_PRESENT);
 }
+#define __pte_needs_invert __pte_needs_invert
 
 /* Get a mask to xor with the page table entry to get the correct pfn. */
 static inline u64 protnone_mask(u64 val)
diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
index 1fba072b3dac..34b12e94b850 100644
--- a/include/linux/pgtable.h
+++ b/include/linux/pgtable.h
@@ -205,6 +205,10 @@  static inline int pmd_young(pmd_t pmd)
 #define arch_flush_lazy_mmu_mode()	do {} while (0)
 #endif
 
+#ifndef __pte_needs_invert
+#define __pte_needs_invert(pte)	false
+#endif
+
 #ifndef set_ptes
 /**
  * set_ptes - Map consecutive pages to a contiguous range of addresses.
@@ -231,7 +235,10 @@  static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
 		if (--nr == 0)
 			break;
 		ptep++;
-		pte = __pte(pte_val(pte) + (1UL << PFN_PTE_SHIFT));
+		if (__pte_needs_invert(pte_val(pte)))
+			pte = __pte(pte_val(pte) - (1UL << PFN_PTE_SHIFT));
+		else
+			pte = __pte(pte_val(pte) + (1UL << PFN_PTE_SHIFT));
 	}
 	arch_leave_lazy_mmu_mode();
 }