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 <[email protected]> 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/[email protected]?part=5
