Re: [PATCH v2 01/15] riscv: introduce raw PTE helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The patch declares raw PTE helpers `__ptep_test_and_clear_young` and `__ptep_set_access_flags` but never defines them, causing a linker error.

Pre-existing issues:
- [High] Non-atomic read-modify-write in `__ptep_set_wrprotect` can overwrite concurrent hardware A/D bit updates, causing silent data corruption.
--

--- Patch [1]: [PATCH v2 01/15] riscv: introduce raw PTE helpers ---
commit 3ca72eca5363bad7487aaf256a6d44b7c83506ca
Author: Yunhui Cui <cuiyunhui@bytedance.com>

riscv: introduce raw PTE helpers

Svnapot needs RISC-V to distinguish between two PTE views: the raw encoded
entries used by architecture code, and the logical per-page PTE view that
will later be exposed to generic MM paths. Split the low-level RISC-V PTE
operations from the public helpers so the raw helpers can keep operating on
the hardware encoding.

> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index a810655ce8f9b..8ca5da9534967 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
[ ... ]
>  #define __HAVE_ARCH_PTEP_SET_ACCESS_FLAGS	/* defined in mm/pgtable.c */
>  extern int ptep_set_access_flags(struct vm_area_struct *vma, unsigned long address,
>  				 pte_t *ptep, pte_t entry, int dirty);
> +int __ptep_set_access_flags(struct vm_area_struct *vma,
> +			    unsigned long address, pte_t *ptep,
> +			    pte_t entry, int dirty);
>  #define __HAVE_ARCH_PTEP_TEST_AND_CLEAR_YOUNG	/* defined in mm/pgtable.c */
>  bool ptep_test_and_clear_young(struct vm_area_struct *vma,
> -		unsigned long address, pte_t *ptep);
> +			       unsigned long address, pte_t *ptep);
> +bool __ptep_test_and_clear_young(struct vm_area_struct *vma,
> +				 unsigned long address, pte_t *ptep);
>  

[Severity: Low]
Are the newly declared __ptep_test_and_clear_young() and
__ptep_set_access_flags() functions implemented anywhere in this patch?

They appear to be declared here but missing definitions, which could cause a
linker error when __ptep_clear_flush_young() calls them below.

[ ... ]
>  #define __HAVE_ARCH_PTEP_SET_WRPROTECT
> -static inline void ptep_set_wrprotect(struct mm_struct *mm,
> -				      unsigned long address, pte_t *ptep)
> +static inline void __ptep_set_wrprotect(struct mm_struct *mm,
> +					unsigned long address, pte_t *ptep)
>  {
>  	pte_t read_pte = READ_ONCE(*ptep);
>  	/*
> @@ -705,9 +784,11 @@ static inline void ptep_set_wrprotect(struct mm_struct *mm,
>  			((pte_val(read_pte) & ~(unsigned long)_PAGE_WRITE) | _PAGE_READ));
>  }

[Severity: High]
This is a pre-existing issue, but does this non-atomic read-modify-write
sequence overwrite concurrent hardware A/D bit updates?

If a concurrent hardware A/D bit update (e.g. via the RISC-V Svadu hardware
page table walker) occurs between the READ_ONCE(*ptep) and the
atomic_long_set(), it seems any bit set by the hardware during this window
would be permanently lost. Could this lead to silent data corruption if a
dirty bit is lost on a writable page during writeback?

This would also propagate to huge pages since pmdp_set_wrprotect()
delegates to this helper.

> +#define __ptep_set_wrprotect __ptep_set_wrprotect
> +
>  #define __HAVE_ARCH_PTEP_CLEAR_YOUNG_FLUSH
> -static inline bool ptep_clear_flush_young(struct vm_area_struct *vma,
> -		unsigned long address, pte_t *ptep)
> +static inline bool __ptep_clear_flush_young(struct vm_area_struct *vma,
> +					    unsigned long address, pte_t *ptep)
>  {
>  	/*
[ ... ]
> -	return ptep_test_and_clear_young(vma, address, ptep);
> +	return __ptep_test_and_clear_young(vma, address, ptep);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784201104.git.cuiyunhui@bytedance.com?part=1