Re: [PATCH RFC 05/11] mm: convert PTE table entries in ptep_get()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect placement of READ_ONCE() around __pte_from_hw() in ptep_get() breaks atomic PTE reads and causes compilation failures for architectures overriding the hook.
- [Medium] ptep_get_lockless() fails to apply the __pte_from_hw() conversion hook when CONFIG_GUP_GET_PXX_LOW_HIGH is enabled.
--

--- Patch [5]: [PATCH RFC 05/11] mm: convert PTE table entries in ptep_get() ---
commit e1a429a7f2c12fbd880800f901c43b4ccbc922d7
Author: Muhammad Usama Anjum <usama.anjum@arm.com>

mm: convert PTE table entries in ptep_get()

This commit introduces a conversion hook __pte_from_hw() for ptep_get(),
allowing the function to accept a pointer to hw_pte_t and return a logical
pte_t value. The generic ptep_get() implementation is updated to use this
new hook.

> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index dad80d264aac2..3003ac9cd81fe 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -493,7 +493,7 @@ static inline int pudp_set_access_flags(struct vm_area_struct *vma,
>  #ifndef ptep_get
>  static inline pte_t ptep_get(hw_pte_t *ptep)
>  {
> -	return READ_ONCE(*ptep);
> +	return READ_ONCE(__pte_from_hw(*ptep));

[Severity: High]
Does wrapping __pte_from_hw() inside READ_ONCE() break the atomic read
semantics and build stability?

READ_ONCE() requires an lvalue to perform a volatile read. If an architecture
implements __pte_from_hw() as an inline function returning an rvalue, this
will cause a compilation error (lvalue required as unary '&' operand).

If implemented as a macro returning a compound literal, it compiles but the
actual read of *ptep occurs outside the volatile scope during the
initialization of the stack temporary. This allows the compiler to tear or
reorder the memory load, potentially breaking the concurrency safety of the
page table read.

Should this be __pte_from_hw(READ_ONCE(*ptep)) instead, to ensure the memory
is read atomically before the conversion is applied?

[Severity: Medium]
Does the lockless fallback loop in ptep_get_lockless() also need to apply
this new __pte_from_hw() conversion hook?

Looking at ptep_get_lockless() in include/linux/pgtable.h for architectures
using CONFIG_GUP_GET_PXX_LOW_HIGH, it currently constructs and returns an
unconverted PTE directly:

	do {
		pte.pte_low = ptep->pte_low;
		smp_rmb();
		pte.pte_high = ptep->pte_high;
		smp_rmb();
	} while (unlikely(pte.pte_low != ptep->pte_low));

	return pte;

If an architecture using this configuration requires a custom hw_pte_t
conversion, callers might misinterpret the logical bits of the unconverted
hardware PTE. Is it necessary to apply __pte_from_hw() to the assembled
PTE before returning it?

>  }
>  #endif
>  
> diff --git a/include/linux/pgtable_types.h b/include/linux/pgtable_types.h
> index ed5dba42a4f8c..61afe27307ec2 100644
> --- a/include/linux/pgtable_types.h
> +++ b/include/linux/pgtable_types.h
> @@ -6,6 +6,7 @@
>  
>  #ifndef __HAVE_ARCH_HW_PTE_T
>  #define hw_pte_t pte_t
> +#define __pte_from_hw(pte)	(pte)
>  #endif
>  
>  #endif /* _LINUX_PGTABLE_TYPES_H */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727164715.2866609-1-usama.anjum@arm.com?part=5