| Message ID | 20240905081124.9576-1-21cnbao@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mm: arm64: document why pte is not advanced in contpte_ptep_set_access_flags() | expand |
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>
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,
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);