Re: [PATCH v1] arm64: mm: Permit PTE SW bits to change in live mappings

[Peter Xu <peterx@redhat.com>]

Hi, Ryan,

On Wed, Jun 19, 2024 at 01:18:56PM +0100, Ryan Roberts wrote:
> Previously pgattr_change_is_safe() was overly-strict and complained
> (e.g. "[  116.262743] __check_safe_pte_update: unsafe attribute change:
> 0x0560000043768fc3 -> 0x0160000043768fc3") if it saw any SW bits change
> in a live PTE. There is no such restriction on SW bits in the Arm ARM.
> 
> Until now, no SW bits have been updated in live mappings via the
> set_ptes() route. PTE_DIRTY would be updated live, but this is handled
> by ptep_set_access_flags() which does not call pgattr_change_is_safe().
> However, with the introduction of uffd-wp for arm64, there is core-mm
> code that does ptep_get(); pte_clear_uffd_wp(); set_ptes(); which
> triggers this false warning.
> 
> Silence this warning by masking out the SW bits during checks.
> 
> The bug isn't technically in the highlighted commit below, but that's
> where bisecting would likely lead as its what made the bug user-visible.
> 
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> Fixes: 5b32510af77b ("arm64/mm: Add uffd write-protect support")
> ---
> 
> Hi All,
> 
> This applies on top of v6.10-rc4 and it would be good to land this as a hotfix
> for v6.10 since its effectively fixing a bug in 5b32510af77b which was merged
> for v6.10.
> 
> I've only been able to trigger this occasionally by running the mm uffd
> selftests, when swap is configured to use a small (64M) zRam disk. With this fix
> applied I can no longer trigger it.

Totally not familiar with the arm64 pgtable checker here, but I'm just
wondering how the swap affected this, as I see there's:

	/* creating or taking down mappings is always safe */
	if (!pte_valid(__pte(old)) || !pte_valid(__pte(new)))
		return true;

Should pte_valid() always report false on swap entries? Does it mean that
it'll always report PASS for anything switch from/to a swap entry for the
checker?

I assume that's also why you didn't cover bit 3 (uffd-wp swap bit on arm64,
per my read in your previous series), but I don't think I'm confident on my
understanding yet.  It might be nice to mention how that was triggered in
the commit message from that regard.

> 
> Thanks,
> Ryan
> 
>  arch/arm64/include/asm/pgtable-hwdef.h | 1 +
>  arch/arm64/mm/mmu.c                    | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/pgtable-hwdef.h b/arch/arm64/include/asm/pgtable-hwdef.h
> index 9943ff0af4c9..1f60aa1bc750 100644
> --- a/arch/arm64/include/asm/pgtable-hwdef.h
> +++ b/arch/arm64/include/asm/pgtable-hwdef.h
> @@ -170,6 +170,7 @@
>  #define PTE_CONT		(_AT(pteval_t, 1) << 52)	/* Contiguous range */
>  #define PTE_PXN			(_AT(pteval_t, 1) << 53)	/* Privileged XN */
>  #define PTE_UXN			(_AT(pteval_t, 1) << 54)	/* User XN */
> +#define PTE_SWBITS_MASK		_AT(pteval_t, (BIT(63) | GENMASK(58, 55)))
> 
>  #define PTE_ADDR_LOW		(((_AT(pteval_t, 1) << (50 - PAGE_SHIFT)) - 1) << PAGE_SHIFT)
>  #ifdef CONFIG_ARM64_PA_BITS_52
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index c927e9312f10..353ea5dc32b8 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -124,7 +124,8 @@ bool pgattr_change_is_safe(u64 old, u64 new)
>  	 * The following mapping attributes may be updated in live
>  	 * kernel mappings without the need for break-before-make.
>  	 */
> -	pteval_t mask = PTE_PXN | PTE_RDONLY | PTE_WRITE | PTE_NG;
> +	pteval_t mask = PTE_PXN | PTE_RDONLY | PTE_WRITE | PTE_NG |
> +			PTE_SWBITS_MASK;

When applying the uffd-wp bit, normally we shouldn't need this as we'll
need to do BBM-alike ops to avoid concurrent HW A/D updates.  E.g.
change_pte_range() uses the ptep_modify_prot_* APIs.

But indeed at least unprotect / clear-uffd-bit doesn't logically need that,
we already do that in e.g. do_wp_page().  From that POV it makes sense to
me, as I also don't see why soft-bits are forbidden to be updated on ptes
if HWs ignore them as a pretty generic concept.  Just want to double check
with you.

> 
>  	/* creating or taking down mappings is always safe */
>  	if (!pte_valid(__pte(old)) || !pte_valid(__pte(new)))
> --
> 2.43.0
> 

When looking at this function I found this and caught my attention too:

	/* live contiguous mappings may not be manipulated at all */
	if ((old | new) & PTE_CONT)
		return false;

I'm now wondering how cont-ptes work with uffd-wp now for arm64, from
either hugetlb or mTHP pov.  This check may be relevant here as a start.

The other thing is since x86 doesn't have cont-ptes yet, uffd-wp didn't
consider that, and there may be things overlooked at least from my side.
E.g., consider wr-protect one cont-pte huge pages on hugetlb:

static inline pte_t huge_pte_mkuffd_wp(pte_t pte)
{
	return huge_pte_wrprotect(pte_mkuffd_wp(pte));
}

I think it means so far it won't touch the rest cont-ptes but the 1st.  Not
sure whether it'll work if write happens on the rest.

For mTHPs, they should still be done in change_pte_range() which doesn't
understand mTHPs yet, so it should loop over all ptes and looks good so
far, but I didn't further check other than that.

Thanks,

-- 
Peter Xu