[PATCH v2] mm: arm64: document why pte is not advanced in contpte_ptep_set_access_flags()

[PATCH v2] mm: arm64: document why pte is not advanced in contpte_ptep_set_access_flags()

[Barry Song]

From: Barry Song <v-songbaohua@oppo.com>

According to David and Ryan, there isn't a bug here, even though we
don't advance the PTE entry, because __ptep_set_access_flags() only
uses the access flags from the entry.

However, we always check pte_same(pte, entry) using the first entry
in __ptep_set_access_flags(). This means that the checks from 1 to
nr - 1 are not comparing the same PTE indexes (thus, they always
return false), which can be a bit confusing. To clarify the code, let's
add some comments.

Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
Signed-off-by: Barry Song <v-songbaohua@oppo.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Will Deacon <will@kernel.org>
---
 -v2:
 * collect Ryan's rb, thanks!
 * doc why but not advance entry
 * refine changelog and subject

 -v1:
 https://lore.kernel.org/linux-mm/20240831083537.62111-1-21cnbao@gmail.com/

 arch/arm64/mm/contpte.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
index a3edced29ac1..55107d27d3f8 100644
--- a/arch/arm64/mm/contpte.c
+++ b/arch/arm64/mm/contpte.c
@@ -421,6 +421,12 @@ int contpte_ptep_set_access_flags(struct vm_area_struct *vma,
 		ptep = contpte_align_down(ptep);
 		start_addr = addr = ALIGN_DOWN(addr, CONT_PTE_SIZE);
 
+		/*
+		 * We are not advancing entry because __ptep_set_access_flags()
+		 * only consumes access flags from entry. And since we have checked
+		 * for the whole contpte block and returned early, pte_same()
+		 * within __ptep_set_access_flags() is likely false.
+		 */
 		for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE)
 			__ptep_set_access_flags(vma, addr, ptep, entry, 0);
 
-- 
2.39.3 (Apple Git-146)

Re: [PATCH v2] mm: arm64: document why pte is not advanced in contpte_ptep_set_access_flags()

[David Hildenbrand]

On 05.09.24 10:11, Barry Song wrote:
> From: Barry Song <v-songbaohua@oppo.com>
> 
> According to David and Ryan, there isn't a bug here, even though we
> don't advance the PTE entry, because __ptep_set_access_flags() only
> uses the access flags from the entry.
> 
> However, we always check pte_same(pte, entry) using the first entry
> in __ptep_set_access_flags(). This means that the checks from 1 to
> nr - 1 are not comparing the same PTE indexes (thus, they always
> return false), which can be a bit confusing. To clarify the code, let's
> add some comments.
> 
> Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Will Deacon <will@kernel.org>
> ---
>   -v2:
>   * collect Ryan's rb, thanks!
>   * doc why but not advance entry
>   * refine changelog and subject
> 
>   -v1:
>   https://lore.kernel.org/linux-mm/20240831083537.62111-1-21cnbao@gmail.com/
> 
>   arch/arm64/mm/contpte.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
> index a3edced29ac1..55107d27d3f8 100644
> --- a/arch/arm64/mm/contpte.c
> +++ b/arch/arm64/mm/contpte.c
> @@ -421,6 +421,12 @@ int contpte_ptep_set_access_flags(struct vm_area_struct *vma,
>   		ptep = contpte_align_down(ptep);
>   		start_addr = addr = ALIGN_DOWN(addr, CONT_PTE_SIZE);
>   
> +		/*
> +		 * We are not advancing entry because __ptep_set_access_flags()
> +		 * only consumes access flags from entry. And since we have checked
> +		 * for the whole contpte block and returned early, pte_same()
> +		 * within __ptep_set_access_flags() is likely false.
> +		 */
>   		for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE)
>   			__ptep_set_access_flags(vma, addr, ptep, entry, 0);
>   

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH v2] mm: arm64: document why pte is not advanced in contpte_ptep_set_access_flags()

[Will Deacon]

On Thu, 05 Sep 2024 20:11:24 +1200, Barry Song wrote:
> According to David and Ryan, there isn't a bug here, even though we
> don't advance the PTE entry, because __ptep_set_access_flags() only
> uses the access flags from the entry.
> 
> However, we always check pte_same(pte, entry) using the first entry
> in __ptep_set_access_flags(). This means that the checks from 1 to
> nr - 1 are not comparing the same PTE indexes (thus, they always
> return false), which can be a bit confusing. To clarify the code, let's
> add some comments.
> 
> [...]

Applied to arm64 (for-next/mm), thanks!

[1/1] mm: arm64: document why pte is not advanced in contpte_ptep_set_access_flags()
      https://git.kernel.org/arm64/c/70565f2be880

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev