quoted

quoted

On Sat, Aug 31, 2024 at 10:06:40PM +1200, Barry Song wrote:

quoted

On Sat, Aug 31, 2024 at 9:54 PM David Hildenbrand [off-list ref] wrote:

quoted

On 31.08.24 10:35, Barry Song wrote:

quoted

From: Barry Song <redacted>

Hi Ryan, David,
it seems contpte_ptep_set_access_flags() has never advanced
pte pfn, and it is setting all entries' pfn to the first
subpage. But I feel quite strange we never have a bug reported.
Am I missing something?

Fixes: 4602e5757bcc ("arm64/mm: wire up PTE_CONT for user mappings")
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 <redacted>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Barry Song <redacted>
---
  arch/arm64/mm/contpte.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
index a3edced29ac1..10dcd2641184 100644
--- a/arch/arm64/mm/contpte.c
+++ b/arch/arm64/mm/contpte.c

@@ -421,8 +421,10 @@ 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);

-             for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE)
+             for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE) {
                      __ptep_set_access_flags(vma, addr, ptep, entry, 0);
+                     entry = pte_advance_pfn(entry, 1);
+             }

              if (dirty)
                      __flush_tlb_range(vma, start_addr, addr,

Taking a closer look at __ptep_set_access_flags(), there is:

/* only preserve the access flags and write permission *
pte_val(entry) &= PTE_RDONLY | PTE_AF | PTE_WRITE | PTE_DIRTY;

So it looks like it doesn't need the PFN?

Correct, I don't believe there is a bug here; __ptep_set_access_flags() only
consumes the access flags from entry.

quoted

quoted

right.

quoted

OTOH, there is the initial:


if (pte_same(pte, entry))
        return 0;

check that might accelerate things.

There is an equivalent check in contpte_ptep_set_access_flags() which is
checking for the whole contpte block and returning early if so. So I don't think
there is a problem here either.

quoted

quoted

quoted

So unless I am missing something, this works as expected? (and if the
pte_same() would frequently be taken with your change would be worthwile
to optimize)

Right. From page 1 to page (nr_pages - 1), we consistently get FALSE
for pte_same().
This seems quite strange. I think we might need to "fix" it, at least
for the sake of code
semantics. on the other hand, if pte_same() is not important, it
should be dropped.

Hi Ryan,
what is your take on this?

The code is correct and working as intended, AFAICT. But I accept that this is
not exactly obvious. I'd be happy to Rb your proposed change if you feel it
clarifies things.

If this is the case, I'd rather add some comments instead in v2?

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);