Enable huge-vmalloc permission change

It is noted at [1] that KFENCE can manipulate kernel pgtable entries during softirqs. It does this by calling set_memory_valid() -> __change_memory_common(). This being a non-sleepable context, we cannot take the init_mm mmap lock. Therefore, add PGWALK_NOLOCK to enable walk_page_range_novma() usage without locks.

[1] https://lore.kernel.org/linux-arm-kernel/[email protected]/

Signed-off-by Dev Jain <[email protected]>

---
 include/linux/pagewalk.h |  2 ++
 mm/pagewalk.c            | 12 ++++++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

include/linux/pagewalk.h +0 -0

index 9700a29f8afb..9bc8853ed3de 100644

include/linux/pagewalk.h +0 -0

 };
 
 /**

mm/pagewalk.c +2 -0

index e478777c86e1..9657cf4664b2 100644
@@ -440,6 +440,8 @@ static inline void process_vma_walk_lock(struct vm_area_struct *vma,
 	case PGWALK_RDLOCK:
 		/* PGWALK_RDLOCK is handled by process_mm_walk_lock */
 		break;
+	default:
+		break;
 	}
 #endif
 }

mm/pagewalk.c +6 -2

-	else
-		mmap_assert_write_locked(walk.mm);
+	if (ops->walk_lock != PGWALK_NOLOCK) {
+		if (mm == &init_mm)
+			mmap_assert_locked(walk.mm);
+		else
+			mmap_assert_write_locked(walk.mm);
+	}
 
 	return walk_pgd_range(start, end, &walk);
 }

Quoted 5 lines · Dev Jain

It is noted at [1] that KFENCE can manipulate kernel pgtable entries during softirqs. It does this by calling set_memory_valid() -> __change_memory_common(). This being a non-sleepable context, we cannot take the init_mm mmap lock. Therefore, add PGWALK_NOLOCK to enable walk_page_range_novma() usage without locks.

It looks like riscv solved this problem by moving from walk_page_range_novma() to apply_to_existing_page_range() in Commit fb1cf0878328 ("riscv: rewrite __kernel_map_pages() to fix sleeping in invalid context"). That won't work for us because the whole point is that we want to support changing permissions on block mappings.

Yang:

Not directly relavent to this patch, but I do worry about the potential need to split the range here though once Yang's series comes in - we would need to allocate memory for pgtables atomically in softirq context. KFENCE is intended to be enabled in production IIRC, so we can't just not allow block mapping when KFENCE is enabled and will likely need to think of a solution for this?

Quoted 15 lines

[1] https://lore.kernel.org/linux-arm-kernel/[email protected]/

Signed-off-by Dev Jain <[email protected]>

---
  include/linux/pagewalk.h |  2 ++
  mm/pagewalk.c            | 12 ++++++++----
  2 files changed, 10 insertions(+), 4 deletions(-)

include/linux/pagewalk.h +0 -0

index 9700a29f8afb..9bc8853ed3de 100644

include/linux/pagewalk.h enum page_walk_lock { · line 18

Quoted 5 lines

include/linux/pagewalk.h +2 -0

@@ -14,6 +14,8 @@ enum page_walk_lock {
  	PGWALK_WRLOCK = 1,
  	/* vma is expected to be already write-locked during the walk */
  	PGWALK_WRLOCK_VERIFY = 2,
+	/* no lock is needed */
+	PGWALK_NOLOCK = 3,

I'd imagine you either want to explicitly forbid this option for the other entrypoints (i.e. the non- _novma variants) or you need to be able to handle this option being passed in to the other functions, which you currently don't do. I'd vote for explcitly disallowing (document it and return error if passed in).

Quoted 17 lines

include/linux/pagewalk.h +0 -0

  };
 
  /**

mm/pagewalk.c +2 -0

index e478777c86e1..9657cf4664b2 100644
@@ -440,6 +440,8 @@ static inline void process_vma_walk_lock(struct vm_area_struct *vma,
  	case PGWALK_RDLOCK:
  		/* PGWALK_RDLOCK is handled by process_mm_walk_lock */
  		break;
+	default:
+		break;
  	}
  #endif
  }

mm/pagewalk.c int walk_page_range_novma(struct mm_struct *mm, unsigned long start, · line 644

Quoted 5 lines

mm/pagewalk.c +0 -2

@@ -640,10 +642,12 @@ int walk_page_range_novma(struct mm_struct *mm, unsigned long start,
  	 * specified address range from being freed. The caller should take
  	 * other actions to prevent this race.
  	 */
-	if (mm == &init_mm)
-		mmap_assert_locked(walk.mm);

Given apply_to_page_range() doesn't do any locking for kernel pgtable walking, I can be convinced that it's also not required for our case using this framework. But why does this framework believe it is necessary? Should the comment above this at least be updated?

Thanks,
Ryan

Quoted 11 lines

mm/pagewalk.c +6 -2

-	else
-		mmap_assert_write_locked(walk.mm);
+	if (ops->walk_lock != PGWALK_NOLOCK) {
+		if (mm == &init_mm)
+			mmap_assert_locked(walk.mm);
+		else
+			mmap_assert_write_locked(walk.mm);
+	}
 
  	return walk_pgd_range(start, end, &walk);
  }

It is noted at [1] that KFENCE can manipulate kernel pgtable entries during softirqs. It does this by calling set_memory_valid() -> __change_memory_common(). This being a non-sleepable context, we cannot take the init_mm mmap lock. Therefore, add PGWALK_NOLOCK to enable walk_page_range_novma() usage without locks.

[1] https://lore.kernel.org/linux-arm-kernel/[email protected]/

Signed-off-by Dev Jain <[email protected]>

---
 include/linux/pagewalk.h |  2 ++
 mm/pagewalk.c            | 12 ++++++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

include/linux/pagewalk.h +0 -0

index 9700a29f8afb..9bc8853ed3de 100644

mm/pagewalk.c +5 -1

+		if (mm == &init_mm)
+			mmap_assert_locked(walk.mm);
+		else
+			mmap_assert_write_locked(walk.mm);
+	}
 
 	return walk_pgd_range(start, end, &walk);
 }
--

2.30.2

+cc Jan for page table stuff.

Quoted 5 lines · Dev Jain

It is noted at [1] that KFENCE can manipulate kernel pgtable entries during softirqs. It does this by calling set_memory_valid() -> __change_memory_common(). This being a non-sleepable context, we cannot take the init_mm mmap lock. Therefore, add PGWALK_NOLOCK to enable walk_page_range_novma() usage without locks.

Hm This is worrying.

You're unconditionally making it possible for dangerous usage here - to walk page tables without a lock.

We need to assert this is only being used in a context where this makes sense, e.g. a no VMA range under the right circumstances.

At the very least we need asserts that we are in a circumstance where this is permitted.

For VMAs, you must keep the VMA stable, which requires a VMA read lock at minimum.

See
https://origin.kernel.org/doc/html/latest/mm/process_addrs.html#page-tables
for details where these requirements are documented.

I also think we should update this documentation to cover off this non-VMA task context stuff. I can perhaps do this so as not to egregiously add workload to this series :)

Also, again this commit message is not enough for such a major change to core mm stuff. I think you need to underline that - in non-task context - you are safe to manipulate _kernel_ mappings, having precluded KFENCE as a concern.

Quoted 2 lines

[1] https://lore.kernel.org/linux-arm-kernel/[email protected]/

Basically expand upon this information.

Basically the commit message refers to your usage, but describes a patch that makes it possible to do unlocked page table walks.

As I get into below, no pun intended, but this needs to be _locked down_ heavily.

- Only walk_page_range_novma() should allow it. All other functions should return -EINVAL if this is set.

- walk_page_range_novma() should assert we're in the appropriate context where this is feasible.

- Comments should be updated accordingly.

- We should assert (at least CONFIG_DEBUG_VM asserts) in every place that checks for a VMA that we are not in this lock mode, since this is disallowed.

Quoted 11 lines

Signed-off-by Dev Jain <[email protected]>

---
  include/linux/pagewalk.h |  2 ++
  mm/pagewalk.c            | 12 ++++++++----
  2 files changed, 10 insertions(+), 4 deletions(-)

include/linux/pagewalk.h +0 -0

index 9700a29f8afb..9bc8853ed3de 100644

include/linux/pagewalk.h enum page_walk_lock { · line 18

Quoted 5 lines

include/linux/pagewalk.h +2 -0

@@ -14,6 +14,8 @@ enum page_walk_lock {
  	PGWALK_WRLOCK = 1,
  	/* vma is expected to be already write-locked during the walk */
  	PGWALK_WRLOCK_VERIFY = 2,
+	/* no lock is needed */
+	PGWALK_NOLOCK = 3,

I'd prefer something very explicitly documenting that, at the very least, this can only be used for non-VMA cases.

It's hard to think of a name here, but the comment should be explicit as to under what circumstances this is allowed.

Quoted 2 lines

include/linux/pagewalk.h +0 -0

  };
 

mm/pagewalk.c static inline void process_vma_walk_lock(struct vm_area_struct *vma, · line 444

Quoted 11 lines

include/linux/pagewalk.h +0 -0

  /**

mm/pagewalk.c +2 -0

index e478777c86e1..9657cf4664b2 100644
@@ -440,6 +440,8 @@ static inline void process_vma_walk_lock(struct vm_area_struct *vma,
  	case PGWALK_RDLOCK:
  		/* PGWALK_RDLOCK is handled by process_mm_walk_lock */
  		break;
+	default:
+		break;

Please no 'default' here, we want to be explicit and cover all cases.

And surely, since you're explicitly only allowing this for non-VMA ranges, this should be a WARN_ON_ONCE() or something?

mm/pagewalk.c int walk_page_range_novma(struct mm_struct *mm, unsigned long start, · line 644

Quoted 8 lines

mm/pagewalk.c +0 -0

  	}
  #endif
  }

@@ -640,10 +642,12 @@ int walk_page_range_novma(struct mm_struct *mm, unsigned long start,
  	 * specified address range from being freed. The caller should take
  	 * other actions to prevent this race.
  	 */

All functions other than this should explicitly disallow this locking mode with -EINVAL checks. I do not want to see this locking mode made available in a broken context.

The full comment:

/*
* 1) For walking the user virtual address space:
*
* The mmap lock protects the page walker from changes to the page
* tables during the walk. However a read lock is insufficient to
* protect those areas which don't have a VMA as munmap() detaches
* the VMAs before downgrading to a read lock and actually tearing
* down PTEs/page tables. In which case, the mmap write lock should
* be hold.
*
* 2) For walking the kernel virtual address space:
*
* The kernel intermediate page tables usually do not be freed, so
* the mmap map read lock is sufficient. But there are some exceptions.
* E.g. memory hot-remove. In which case, the mmap lock is insufficient
* to prevent the intermediate kernel pages tables belonging to the
* specified address range from being freed. The caller should take
* other actions to prevent this race.
*/

Are you walking kernel memory only? Point 1 above explicitly points out why userland novma memory requires a lock.

For point 2 you need to indicate why you don't need to consider hotplugging,

etc.

But as Ryan points out elsewhere, you should be expanding this comment to explain your case...

You should also assert you're in a context where this applies and error out/WARN if not.

mm/pagewalk.c int walk_page_range_novma(struct mm_struct *mm, unsigned long start, · line 645

Quoted 5 lines

mm/pagewalk.c +1 -4

-	if (mm == &init_mm)
-		mmap_assert_locked(walk.mm);
-	else
-		mmap_assert_write_locked(walk.mm);
+	if (ops->walk_lock != PGWALK_NOLOCK) {

I really don't like the idea that you're allowing no lock for userland mappings.

This should at the very least be:

if (mm == &init_mm)  {
	if (ops->walk_lock != PGWALK_NOLOCK)
		mmap_assert_locked(walk.mm);
} else {
	mmap_assert_write_locked(walk.mm);
}

Quoted 10 lines

mm/pagewalk.c +5 -1

+		if (mm == &init_mm)
+			mmap_assert_locked(walk.mm);
+		else
+			mmap_assert_write_locked(walk.mm);
+	}
 
  	return walk_pgd_range(start, end, &walk);
  }
--

Quoted 2 lines

2.30.2

We have to be _really_ careful with this stuff. It's very fiddly and brokenness can be a security issue.

Quoted 27 lines · Lorenzo Stoakes

+cc Jan for page table stuff.

Quoted 5 lines · Dev Jain

It is noted at [1] that KFENCE can manipulate kernel pgtable entries during softirqs. It does this by calling set_memory_valid() -> __change_memory_common(). This being a non-sleepable context, we cannot take the init_mm mmap lock. Therefore, add PGWALK_NOLOCK to enable walk_page_range_novma() usage without locks.

Hm This is worrying.

You're unconditionally making it possible for dangerous usage here - to walk page tables without a lock.

We need to assert this is only being used in a context where this makes sense, e.g. a no VMA range under the right circumstances.

At the very least we need asserts that we are in a circumstance where this is permitted.

For VMAs, you must keep the VMA stable, which requires a VMA read lock at minimum.

See
https://origin.kernel.org/doc/html/latest/mm/process_addrs.html#page-tables

for details where these requirements are documented.

Quoted 8 lines

I also think we should update this documentation to cover off this non-VMA task context stuff. I can perhaps do this so as not to egregiously add workload to this series :)

Also, again this commit message is not enough for such a major change to core mm stuff. I think you need to underline that - in non-task context - you are safe to manipulate _kernel_ mappings, having precluded KFENCE as a concern.

Sorry for late reply, after your comments I had to really go and understand kernel pagetable walking properly by reading your process_addrs documentation and reading the code, so that I could prepare an answer and improve my understanding, thanks for your review!

How does the below comment above PGWALK_NOLOCK look?

"Walk without any lock. Use of this is only meant for the case where there is no underlying VMA, and the user has exclusive control over the range, guaranteeing no concurrent access. For example, changing permissions of vmalloc objects."

and the patch description can be modified as
"
It is noted at [1] that KFENCE can manipulate kernel pgtable entries during softirqs. It does this by calling set_memory_valid() -> __change_memory_common(). This being a non-sleepable context, we cannot take the init_mm mmap lock. Therefore, add PGWALK_NOLOCK to enable walk_page_range_novma() usage without locks.
Currently, apply_to_page_range is being used by __change_memory_common() to change permissions over a range of vmalloc space, without any locking. Patch 2 in this series shifts to the usage of walk_page_range_novma(), hence this patch is needed. We do not need any locks because the vmalloc object has exclusive access to the range, i.e two vmalloc objects do not share the same physical address.
"

Quoted 7 lines

Quoted 2 lines

[1] https://lore.kernel.org/linux-arm-kernel/[email protected]/

Basically expand upon this information.

Basically the commit message refers to your usage, but describes a patch that makes it possible to do unlocked page table walks.

Quoted 6 lines

As I get into below, no pun intended, but this needs to be _locked down_ heavily.

- Only walk_page_range_novma() should allow it. All other functions should
   return -EINVAL if this is set.

Sure.

Quoted 1 line

Quoted 2 lines

- walk_page_range_novma() should assert we're in the appropriate context where this is feasible.

There should be two conditions: that the mm is init_mm, and the start address belongs to the vmalloc (or module) space. I am a little nervous about the second. On searching throughout the codebase, I could find only vmalloc and module addresses getting modified through set_memory_* API, but I couldn't prove that all such usages are being done on vmalloc/module addresses.

Quoted 33 lines

- Comments should be updated accordingly.

- We should assert (at least CONFIG_DEBUG_VM asserts) in every place that
   checks for a VMA that we are not in this lock mode, since this is disallowed.

Quoted 11 lines

Signed-off-by Dev Jain <[email protected]>

---
  include/linux/pagewalk.h |  2 ++
  mm/pagewalk.c            | 12 ++++++++----

  2 files changed, 10 insertions(+), 4 deletions(-)

include/linux/pagewalk.h +0 -0

index 9700a29f8afb..9bc8853ed3de 100644

include/linux/pagewalk.h enum page_walk_lock { · line 18

Quoted 5 lines

include/linux/pagewalk.h +2 -0

@@ -14,6 +14,8 @@ enum page_walk_lock {
       PGWALK_WRLOCK = 1,
       /* vma is expected to be already write-locked during the walk */
       PGWALK_WRLOCK_VERIFY = 2,
+    /* no lock is needed */
+    PGWALK_NOLOCK = 3,

I'd prefer something very explicitly documenting that, at the very least, this can only be used for non-VMA cases.

It's hard to think of a name here, but the comment should be explicit as to under what circumstances this is allowed.

Quoted 2 lines

include/linux/pagewalk.h +0 -0

   };
 

Quoted 13 lines

Quoted 7 lines

include/linux/pagewalk.h +0 -0

   /**

mm/pagewalk.c +0 -0

index e478777c86e1..9657cf4664b2 100644
@@ -440,6 +440,8 @@ static inline void process_vma_walk_lock(struct 
 vm_area_struct *vma,

Quoted 5 lines

      case PGWALK_RDLOCK:
          /* PGWALK_RDLOCK is handled by process_mm_walk_lock */ break;
+    default:
+        break;

Please no 'default' here, we want to be explicit and cover all cases.

Sure.

Quoted 3 lines

And surely, since you're explicitly only allowing this for non-VMA ranges, this should be a WARN_ON_ONCE() or something?

Sounds good, maybe a WARN_ON_ONCE(vma)?

Quoted 35 lines

mm/pagewalk.c int walk_page_range_novma(struct mm_struct · line 645

Quoted 9 lines

      }
  #endif
  }

mm/pagewalk.c +0 -0

@@ -640,10 +642,12 @@ int walk_page_range_novma(struct mm_struct 
 *mm, unsigned long start,
        * specified address range from being freed. The caller should take
        * other actions to prevent this race.
        */

All functions other than this should explicitly disallow this locking mode with -EINVAL checks. I do not want to see this locking mode made available in a broken context.

The full comment:

    /*
     * 1) For walking the user virtual address space:
     *
     * The mmap lock protects the page walker from changes to the page
     * tables during the walk.  However a read lock is insufficient to
     * protect those areas which don't have a VMA as munmap() detaches
     * the VMAs before downgrading to a read lock and actually tearing
     * down PTEs/page tables. In which case, the mmap write lock should
     * be hold.
     *
     * 2) For walking the kernel virtual address space:
     *
     * The kernel intermediate page tables usually do not be freed, so
     * the mmap map read lock is sufficient. But there are some exceptions.
     * E.g. memory hot-remove. In which case, the mmap lock is insufficient
     * to prevent the intermediate kernel pages tables belonging to the
     * specified address range from being freed. The caller should take
     * other actions to prevent this race.
     */

Quoted 4 lines

Are you walking kernel memory only? Point 1 above explicitly points out why userland novma memory requires a lock.

For point 2 you need to indicate why you don't need to consider hotplugging,

Well, hotunplugging will first offline the physical memory, and since the vmalloc object has the reference to the pages, there is no race.

Quoted 15 lines

etc.

But as Ryan points out elsewhere, you should be expanding this comment to explain your case...

You should also assert you're in a context where this applies and error out/WARN if not.

mm/pagewalk.c int walk_page_range_novma(struct mm_struct *mm, unsigned long start, · line 645

Quoted 5 lines

mm/pagewalk.c +1 -4

-    if (mm == &init_mm)
-        mmap_assert_locked(walk.mm);
-    else
-        mmap_assert_write_locked(walk.mm);
+    if (ops->walk_lock != PGWALK_NOLOCK) {

I really don't like the idea that you're allowing no lock for userland mappings.

Quoted 8 lines

This should at the very least be:

if (mm == &init_mm)  {
    if (ops->walk_lock != PGWALK_NOLOCK)
        mmap_assert_locked(walk.mm);
} else {
    mmap_assert_write_locked(walk.mm);
}

Sure.

Quoted 14 lines

Quoted 8 lines

mm/pagewalk.c +5 -0

+        if (mm == &init_mm)
+            mmap_assert_locked(walk.mm);
+        else
+            mmap_assert_write_locked(walk.mm);
+    }
 
       return walk_pgd_range(start, end, &walk);

Quoted 4 lines

  }

Signature · 1 line

2.30.2

We have to be _really_ careful with this stuff. It's very fiddly and brokenness can be a security issue.

Quoted 26 lines · Lorenzo Stoakes

+cc Jan for page table stuff.

Quoted 5 lines · Dev Jain

It is noted at [1] that KFENCE can manipulate kernel pgtable entries during softirqs. It does this by calling set_memory_valid() -> __change_memory_common(). This being a non-sleepable context, we cannot take the init_mm mmap lock. Therefore, add PGWALK_NOLOCK to enable walk_page_range_novma() usage without locks.

Hm This is worrying.

You're unconditionally making it possible for dangerous usage here - to walk page tables without a lock.

We need to assert this is only being used in a context where this makes sense, e.g. a no VMA range under the right circumstances.

At the very least we need asserts that we are in a circumstance where this is permitted.

For VMAs, you must keep the VMA stable, which requires a VMA read lock at minimum.

See
https://origin.kernel.org/doc/html/latest/mm/process_addrs.html#page-tables
for details where these requirements are documented.

Quoted 8 lines

I also think we should update this documentation to cover off this non-VMA task context stuff. I can perhaps do this so as not to egregiously add workload to this series :)

Also, again this commit message is not enough for such a major change to core mm stuff. I think you need to underline that - in non-task context - you are safe to manipulate _kernel_ mappings, having precluded KFENCE as a concern.

Sorry for late reply, after your comments I had to really go and understand kernel pagetable walking properly by reading your process_addrs documentation and reading the code, so that I could prepare an answer and improve my understanding, thanks for your review!

How does the below comment above PGWALK_NOLOCK look?

"Walk without any lock. Use of this is only meant for the
case where there is no underlying VMA, and the user has exclusive control over the range, guaranteeing no concurrent access. For example, changing permissions of vmalloc objects."

and the patch description can be modified as
"
It is noted at [1] that KFENCE can manipulate kernel pgtable entries during softirqs. It does this by calling set_memory_valid() -> __change_memory_common(). This being a non-sleepable context, we cannot take the init_mm mmap lock. Therefore, add PGWALK_NOLOCK to enable walk_page_range_novma() usage without

locks.
Currently, apply_to_page_range is being used by __change_memory_common() to change permissions over a range of vmalloc space, without any locking. Patch 2 in this series shifts to the usage of walk_page_range_novma(), hence this patch is needed. We do not need any locks because the vmalloc object has exclusive access to the range, i.e two vmalloc objects do not share the same physical address.
"

Quoted 6 lines

Quoted 4 lines

[1] https://lore.kernel.org/linux-arm-kernel/[email protected]/

Basically expand upon this information.

Quoted 8 lines

Basically the commit message refers to your usage, but describes a patch that makes it possible to do unlocked page table walks.

As I get into below, no pun intended, but this needs to be _locked down_ heavily.

- Only walk_page_range_novma() should allow it. All other functions should return -EINVAL if this is set.

Sure.

Quoted 1 line

Quoted 2 lines

- walk_page_range_novma() should assert we're in the appropriate context where this is feasible.

There should be two conditions: that the mm is init_mm, and the start address belongs to the vmalloc (or module) space. I am a little nervous about the second. On searching throughout the codebase, I could find only vmalloc and module addresses getting modified through set_memory_* API, but I couldn't prove that all such usages are being done on vmalloc/module addresses.

Quoted 32 lines

- Comments should be updated accordingly.

- We should assert (at least CONFIG_DEBUG_VM asserts) in every place that checks for a VMA that we are not in this lock mode, since this is disallowed.

Quoted 11 lines

Signed-off-by Dev Jain <[email protected]>

---
  include/linux/pagewalk.h |  2 ++
  mm/pagewalk.c            | 12 ++++++++----
  2 files changed, 10 insertions(+), 4 deletions(-)

include/linux/pagewalk.h +0 -0

index 9700a29f8afb..9bc8853ed3de 100644

include/linux/pagewalk.h enum page_walk_lock { · line 18

Quoted 5 lines

include/linux/pagewalk.h +2 -0

@@ -14,6 +14,8 @@ enum page_walk_lock {
  	PGWALK_WRLOCK = 1,
  	/* vma is expected to be already write-locked during the walk */
  	PGWALK_WRLOCK_VERIFY = 2,
+	/* no lock is needed */
+	PGWALK_NOLOCK = 3,

I'd prefer something very explicitly documenting that, at the very least, this can only be used for non-VMA cases.

It's hard to think of a name here, but the comment should be explicit as to under what circumstances this is allowed.

Quoted 2 lines

include/linux/pagewalk.h +0 -0

  };
 

Quoted 12 lines

mm/pagewalk.c static inline void process_vma_walk_lock(struct vm_area_struct *vma, · line 444

Quoted 11 lines

include/linux/pagewalk.h +0 -0

  /**

mm/pagewalk.c +2 -0

index e478777c86e1..9657cf4664b2 100644
@@ -440,6 +440,8 @@ static inline void process_vma_walk_lock(struct vm_area_struct *vma,
  	case PGWALK_RDLOCK:
  		/* PGWALK_RDLOCK is handled by process_mm_walk_lock */
  		break;
+	default:
+		break;

Please no 'default' here, we want to be explicit and cover all cases.

Sure.

Quoted 3 lines

And surely, since you're explicitly only allowing this for non-VMA ranges, this should be a WARN_ON_ONCE() or something?

Sounds good, maybe a WARN_ON_ONCE(vma)?

Quoted 34 lines

mm/pagewalk.c int walk_page_range_novma(struct mm_struct *mm, unsigned long start, · line 644

Quoted 8 lines

mm/pagewalk.c +0 -0

  	}
  #endif
  }

@@ -640,10 +642,12 @@ int walk_page_range_novma(struct mm_struct *mm, unsigned long start,
  	 * specified address range from being freed. The caller should take
  	 * other actions to prevent this race.
  	 */

All functions other than this should explicitly disallow this locking mode with -EINVAL checks. I do not want to see this locking mode made available in a broken context.

The full comment:

/*
* 1) For walking the user virtual address space:
*
* The mmap lock protects the page walker from changes to the page
* tables during the walk. However a read lock is insufficient to
* protect those areas which don't have a VMA as munmap() detaches
* the VMAs before downgrading to a read lock and actually tearing
* down PTEs/page tables. In which case, the mmap write lock should
* be hold.
*
* 2) For walking the kernel virtual address space:
*
* The kernel intermediate page tables usually do not be freed, so
* the mmap map read lock is sufficient. But there are some exceptions.
* E.g. memory hot-remove. In which case, the mmap lock is insufficient
* to prevent the intermediate kernel pages tables belonging to the
* specified address range from being freed. The caller should take
* other actions to prevent this race.
*/

Quoted 4 lines

Are you walking kernel memory only? Point 1 above explicitly points out why userland novma memory requires a lock.

For point 2 you need to indicate why you don't need to consider hotplugging,

Well, hotunplugging will first offline the physical memory, and since the vmalloc object has the reference to the pages, there is no race.

Quoted 16 lines

etc.

But as Ryan points out elsewhere, you should be expanding this comment to explain your case...

You should also assert you're in a context where this applies and error out/WARN if not.

mm/pagewalk.c int walk_page_range_novma(struct mm_struct *mm, unsigned long start, · line 645

Quoted 5 lines

mm/pagewalk.c +1 -4

-	if (mm == &init_mm)
-		mmap_assert_locked(walk.mm);
-	else
-		mmap_assert_write_locked(walk.mm);
+	if (ops->walk_lock != PGWALK_NOLOCK) {

I really don't like the idea that you're allowing no lock for userland mappings.

Quoted 8 lines

This should at the very least be:

if (mm == &init_mm)  {
	if (ops->walk_lock != PGWALK_NOLOCK)
		mmap_assert_locked(walk.mm);
} else {
	mmap_assert_write_locked(walk.mm);
}

Sure.

Quoted 14 lines

Quoted 10 lines

mm/pagewalk.c +5 -1

+		if (mm == &init_mm)
+			mmap_assert_locked(walk.mm);
+		else
+			mmap_assert_write_locked(walk.mm);
+	}
 
  	return walk_pgd_range(start, end, &walk);
  }
--

Quoted 2 lines

2.30.2

We have to be _really_ careful with this stuff. It's very fiddly and brokenness can be a security issue.

Move away from apply_to_page_range(), which does not honour leaf mappings, to walk_page_range_novma(). The callbacks emit a warning and return EINVAL if a partial range is detected.

Signed-off-by Dev Jain <[email protected]>

---
 arch/arm64/mm/pageattr.c | 69 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 64 insertions(+), 5 deletions(-)

arch/arm64/mm/pageattr.c +1 -0

index 39fd1f7ff02a..a5c829c64969 100644
@@ -8,6 +8,7 @@
 #include <linux/mem_encrypt.h>
 #include <linux/sched.h>
 #include <linux/vmalloc.h>
+#include <linux/pagewalk.h>
 
 #include <asm/cacheflush.h>
 #include <asm/pgtable-prot.h>

arch/arm64/mm/pageattr.c +27 -0

+
+static int pageattr_pud_entry(pud_t *pud, unsigned long addr,
+			      unsigned long next, struct mm_walk *walk)
+{
+	pud_t val = pudp_get(pud);
+
+	if (pud_leaf(val)) {
+		if (WARN_ON_ONCE((next - addr) != PUD_SIZE))
+			return -EINVAL;
+		val = __pud(set_pageattr_masks(pud_val(val), walk));
+		set_pud(pud, val);
+		walk->action = ACTION_CONTINUE;
+	}
+
+	return 0;
+}
+
+static int pageattr_pmd_entry(pmd_t *pmd, unsigned long addr,
+			      unsigned long next, struct mm_walk *walk)
+{
+	pmd_t val = pmdp_get(pmd);
+
+	if (pmd_leaf(val)) {
+		if (WARN_ON_ONCE((next - addr) != PMD_SIZE))
+			return -EINVAL;
+		val = __pmd(set_pageattr_masks(pmd_val(val), walk));
+		set_pmd(pmd, val);

arch/arm64/mm/pageattr.c +3 -3

+	.walk_lock	= PGWALK_NOLOCK,
+};
+
 bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED);
 
 bool can_set_direct_map(void)

@@ -49,9 +111,6 @@ static int change_page_range(pte_t *ptep, unsigned long addr, void *data)
 	return 0;
 }
 
-/*
- * This function assumes that the range is mapped with PAGE_SIZE pages.
- */
 static int __change_memory_common(unsigned long start, unsigned long size,
 				pgprot_t set_mask, pgprot_t clear_mask)
 {

Quoted 3 lines · Dev Jain

Move away from apply_to_page_range(), which does not honour leaf mappings, to walk_page_range_novma(). The callbacks emit a warning and return EINVAL if a partial range is detected.

Perhaps:

"""
apply_to_page_range(), which was previously used to change permissions for kernel mapped memory, can only operate on page mappings. In future we want to support block mappings for more efficient TLB usage. Reimplement pageattr.c to instead use walk_page_range_novma() to visit and modify leaf mappings of all sizes.

We only require that the start and end of a given range lie on leaf mapping boundaries. If this is not the case, emit a warning and return -EINVAL. """

Quoted 22 lines

Signed-off-by Dev Jain <[email protected]>

---
  arch/arm64/mm/pageattr.c | 69 +++++++++++++++++++++++++++++++++++++---
  1 file changed, 64 insertions(+), 5 deletions(-)

arch/arm64/mm/pageattr.c +1 -0

index 39fd1f7ff02a..a5c829c64969 100644
@@ -8,6 +8,7 @@
  #include <linux/mem_encrypt.h>
  #include <linux/sched.h>
  #include <linux/vmalloc.h>
+#include <linux/pagewalk.h>
 
  #include <asm/cacheflush.h>
  #include <asm/pgtable-prot.h>

@@ -20,6 +21,67 @@ struct page_change_data {
  	pgprot_t clear_mask;
  };
 

arch/arm64/mm/pageattr.c struct page_change_data { · line 24

Quoted 1 line

arch/arm64/mm/pageattr.c +1 -0

+static pteval_t set_pageattr_masks(unsigned long val, struct mm_walk *walk)

Please don't use unsigned long for raw ptes; This will break with D128 pgtables.

Anshuman had a patch in flight to introduce ptdesc_t as a generic/any level raw value. It would be preferable to incorporate that patch and use it. pteval_t isn't really correct because this is for any level and that implies pte level only.

arch/arm64/mm/pageattr.c struct page_change_data { · line 27

Quoted 4 lines

arch/arm64/mm/pageattr.c +3 -0

+{
+	struct page_change_data *masks = walk->private;
+	unsigned long new_val = val;

why do you need new_val? Why not just update and return val?

arch/arm64/mm/pageattr.c struct page_change_data { · line 33

Quoted 7 lines

arch/arm64/mm/pageattr.c +6 -0

+
+	new_val &= ~(pgprot_val(masks->clear_mask));
+	new_val |= (pgprot_val(masks->set_mask));
+
+	return new_val;
+}

One potential pitfall of having a generic function that can change the permissions for ptes at all levels is that bit 1 is defined differently for level 3 vs the other levels. I don't think there should be any issues here in practice having had a quick look at all the masks that users currently pass in though.

Quoted 27 lines

arch/arm64/mm/pageattr.c +27 -0

+
+static int pageattr_pud_entry(pud_t *pud, unsigned long addr,
+			      unsigned long next, struct mm_walk *walk)
+{
+	pud_t val = pudp_get(pud);
+
+	if (pud_leaf(val)) {
+		if (WARN_ON_ONCE((next - addr) != PUD_SIZE))
+			return -EINVAL;
+		val = __pud(set_pageattr_masks(pud_val(val), walk));
+		set_pud(pud, val);
+		walk->action = ACTION_CONTINUE;
+	}
+
+	return 0;
+}
+
+static int pageattr_pmd_entry(pmd_t *pmd, unsigned long addr,
+			      unsigned long next, struct mm_walk *walk)
+{
+	pmd_t val = pmdp_get(pmd);
+
+	if (pmd_leaf(val)) {
+		if (WARN_ON_ONCE((next - addr) != PMD_SIZE))
+			return -EINVAL;
+		val = __pmd(set_pageattr_masks(pmd_val(val), walk));
+		set_pmd(pmd, val);

arch/arm64/mm/pageattr.c struct page_change_data { · line 70

Quoted 10 lines

arch/arm64/mm/pageattr.c +10 -0

+		walk->action = ACTION_CONTINUE;
+	}
+
+	return 0;
+}
+
+static int pageattr_pte_entry(pte_t *pte, unsigned long addr,
+			      unsigned long next, struct mm_walk *walk)
+{
+	pte_t val = ptep_get(pte);

BUG: Use __ptep_get(), which is "below" the contpte management layer. ptep_get()

will look at the contiguous bit and potentially decide to accumulate all the a/d bits in the block which is not relavent for kernel mappings.

arch/arm64/mm/pageattr.c struct page_change_data { · line 73

Quoted 4 lines

arch/arm64/mm/pageattr.c +3 -0

+
+	val = __pte(set_pageattr_masks(pte_val(val), walk));
+	set_pte(pte, val);

BUG: Use __set_pte(). Same reasoning as above. But this is more harmful because

set_pte() will try to detect contpte blocks and may zero/flush the entries. Which would be very bad for kernel mappings.

arch/arm64/mm/pageattr.c struct page_change_data { · line 81

Quoted 9 lines

arch/arm64/mm/pageattr.c +8 -0

+
+	return 0;
+}
+
+static const struct mm_walk_ops pageattr_ops = {
+	.pud_entry	= pageattr_pud_entry,
+	.pmd_entry	= pageattr_pmd_entry,
+	.pte_entry	= pageattr_pte_entry,

Is there a reason why you don't have pgd and p4d entries? I think there are configs where the pgd may contain leaf mappings. Possibly 64K/42-bit, which will have 2 levels and I think they will be pgd and pte. So I think you'd better implement all levels to be correct.

Quoted 18 lines

arch/arm64/mm/pageattr.c +3 -3

+	.walk_lock	= PGWALK_NOLOCK,
+};
+
  bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED);
 
  bool can_set_direct_map(void)

@@ -49,9 +111,6 @@ static int change_page_range(pte_t *ptep, unsigned long addr, void *data)
  	return 0;
  }
 
-/*
- * This function assumes that the range is mapped with PAGE_SIZE pages.
- */
  static int __change_memory_common(unsigned long start, unsigned long size,
  				pgprot_t set_mask, pgprot_t clear_mask)
  {

arch/arm64/mm/pageattr.c static int __change_memory_common(unsigned long start, unsigned long size, · line 127

Quoted 10 lines

arch/arm64/mm/pageattr.c +2 -2

@@ -61,8 +120,8 @@ static int __change_memory_common(unsigned long start, unsigned long size,
  	data.set_mask = set_mask;
  	data.clear_mask = clear_mask;
 
-	ret = apply_to_page_range(&init_mm, start, size, change_page_range,
-					&data);
+	ret = walk_page_range_novma(&init_mm, start, start + size,
+				    &pageattr_ops, NULL, &data);
 
  	/*
  	 * If the memory is being made valid without changing any other bits

I notice that set_direct_map_invalid_noflush() and set_direct_map_default_noflush() don't use __change_memory_common() but instead call apply_to_page_range() direct. (presumably because they don't want the associated tlb flush). Is there a reason not to update these callers too?

Perhaps it would be cleaner to wrap in ___change_memory_common (3 leading underscores) which does everything except the flush).

Thanks,
Ryan

Move away from apply_to_page_range(), which does not honour leaf mappings, to walk_page_range_novma(). The callbacks emit a warning and return EINVAL if a partial range is detected.

Signed-off-by Dev Jain <[email protected]>

---
 arch/arm64/mm/pageattr.c | 69 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 64 insertions(+), 5 deletions(-)

arch/arm64/mm/pageattr.c +64 -6

index 39fd1f7ff02a..a5c829c64969 100644
@@ -8,6 +8,7 @@
 #include <linux/mem_encrypt.h>
 #include <linux/sched.h>
 #include <linux/vmalloc.h>
+#include <linux/pagewalk.h>
 
 #include <asm/cacheflush.h>
 #include <asm/pgtable-prot.h>

@@ -20,6 +21,67 @@ struct page_change_data {
 	pgprot_t clear_mask;
 };
 
+static pteval_t set_pageattr_masks(unsigned long val, struct mm_walk *walk)
+{
+	struct page_change_data *masks = walk->private;
+	unsigned long new_val = val;
+
+	new_val &= ~(pgprot_val(masks->clear_mask));
+	new_val |= (pgprot_val(masks->set_mask));
+
+	return new_val;
+}
+
+static int pageattr_pud_entry(pud_t *pud, unsigned long addr,
+			      unsigned long next, struct mm_walk *walk)
+{
+	pud_t val = pudp_get(pud);
+
+	if (pud_leaf(val)) {
+		if (WARN_ON_ONCE((next - addr) != PUD_SIZE))
+			return -EINVAL;
+		val = __pud(set_pageattr_masks(pud_val(val), walk));
+		set_pud(pud, val);
+		walk->action = ACTION_CONTINUE;
+	}
+
+	return 0;
+}
+
+static int pageattr_pmd_entry(pmd_t *pmd, unsigned long addr,
+			      unsigned long next, struct mm_walk *walk)
+{
+	pmd_t val = pmdp_get(pmd);
+
+	if (pmd_leaf(val)) {
+		if (WARN_ON_ONCE((next - addr) != PMD_SIZE))
+			return -EINVAL;
+		val = __pmd(set_pageattr_masks(pmd_val(val), walk));
+		set_pmd(pmd, val);
+		walk->action = ACTION_CONTINUE;
+	}
+
+	return 0;
+}
+
+static int pageattr_pte_entry(pte_t *pte, unsigned long addr,
+			      unsigned long next, struct mm_walk *walk)
+{
+	pte_t val = ptep_get(pte);
+
+	val = __pte(set_pageattr_masks(pte_val(val), walk));
+	set_pte(pte, val);
+
+	return 0;
+}
+
+static const struct mm_walk_ops pageattr_ops = {
+	.pud_entry	= pageattr_pud_entry,
+	.pmd_entry	= pageattr_pmd_entry,
+	.pte_entry	= pageattr_pte_entry,
+	.walk_lock	= PGWALK_NOLOCK,
+};
+
 bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED);
 
 bool can_set_direct_map(void)

@@ -49,9 +111,6 @@ static int change_page_range(pte_t *ptep, unsigned long addr, void *data)
 	return 0;
 }
 
-/*
- * This function assumes that the range is mapped with PAGE_SIZE pages.
- */
 static int __change_memory_common(unsigned long start, unsigned long size,
 				pgprot_t set_mask, pgprot_t clear_mask)
 {

@@ -61,8 +120,8 @@ static int __change_memory_common(unsigned long start, unsigned long size,
 	data.set_mask = set_mask;
 	data.clear_mask = clear_mask;
 
-	ret = apply_to_page_range(&init_mm, start, size, change_page_range,
-					&data);
+	ret = walk_page_range_novma(&init_mm, start, start + size,
+				    &pageattr_ops, NULL, &data);
 
 	/*
 	 * If the memory is being made valid without changing any other bits
--

2.30.2

Quoted 3 lines · Dev Jain

Move away from apply_to_page_range(), which does not honour leaf mappings, to walk_page_range_novma(). The callbacks emit a warning and return EINVAL if a partial range is detected.

Hm a follow up question here - why not just improve apply_to_page_range() to honour leaf mappings?

What does honouring leaf mappings actually mean? You mean handling huge pages?

Would it be all that difficult to implement?

It seems like you're pushing a bunch of the 'applying' logic over from there to a walker that isn't maybe best suited to it and having to introduce an iffy new form of locking...

Can we go vice-versa? :)

Also obviously walk_page_range_novma() doesn't exist any more :P walk_kernel_page_table_range() is the preferred solution.

Quoted 109 lines

Signed-off-by Dev Jain <[email protected]>

---
  arch/arm64/mm/pageattr.c | 69 +++++++++++++++++++++++++++++++++++++---
  1 file changed, 64 insertions(+), 5 deletions(-)

arch/arm64/mm/pageattr.c +64 -6

index 39fd1f7ff02a..a5c829c64969 100644
@@ -8,6 +8,7 @@
  #include <linux/mem_encrypt.h>
  #include <linux/sched.h>
  #include <linux/vmalloc.h>
+#include <linux/pagewalk.h>
 
  #include <asm/cacheflush.h>
  #include <asm/pgtable-prot.h>

@@ -20,6 +21,67 @@ struct page_change_data {
  	pgprot_t clear_mask;
  };
 
+static pteval_t set_pageattr_masks(unsigned long val, struct mm_walk *walk)
+{
+	struct page_change_data *masks = walk->private;
+	unsigned long new_val = val;
+
+	new_val &= ~(pgprot_val(masks->clear_mask));
+	new_val |= (pgprot_val(masks->set_mask));
+
+	return new_val;
+}
+
+static int pageattr_pud_entry(pud_t *pud, unsigned long addr,
+			      unsigned long next, struct mm_walk *walk)
+{
+	pud_t val = pudp_get(pud);
+
+	if (pud_leaf(val)) {
+		if (WARN_ON_ONCE((next - addr) != PUD_SIZE))
+			return -EINVAL;
+		val = __pud(set_pageattr_masks(pud_val(val), walk));
+		set_pud(pud, val);
+		walk->action = ACTION_CONTINUE;
+	}
+
+	return 0;
+}
+
+static int pageattr_pmd_entry(pmd_t *pmd, unsigned long addr,
+			      unsigned long next, struct mm_walk *walk)
+{
+	pmd_t val = pmdp_get(pmd);
+
+	if (pmd_leaf(val)) {
+		if (WARN_ON_ONCE((next - addr) != PMD_SIZE))
+			return -EINVAL;
+		val = __pmd(set_pageattr_masks(pmd_val(val), walk));
+		set_pmd(pmd, val);
+		walk->action = ACTION_CONTINUE;
+	}
+
+	return 0;
+}
+
+static int pageattr_pte_entry(pte_t *pte, unsigned long addr,
+			      unsigned long next, struct mm_walk *walk)
+{
+	pte_t val = ptep_get(pte);
+
+	val = __pte(set_pageattr_masks(pte_val(val), walk));
+	set_pte(pte, val);
+
+	return 0;
+}
+
+static const struct mm_walk_ops pageattr_ops = {
+	.pud_entry	= pageattr_pud_entry,
+	.pmd_entry	= pageattr_pmd_entry,
+	.pte_entry	= pageattr_pte_entry,
+	.walk_lock	= PGWALK_NOLOCK,
+};
+
  bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED);
 
  bool can_set_direct_map(void)

@@ -49,9 +111,6 @@ static int change_page_range(pte_t *ptep, unsigned long addr, void *data)
  	return 0;
  }
 
-/*
- * This function assumes that the range is mapped with PAGE_SIZE pages.
- */
  static int __change_memory_common(unsigned long start, unsigned long size,
  				pgprot_t set_mask, pgprot_t clear_mask)
  {

@@ -61,8 +120,8 @@ static int __change_memory_common(unsigned long start, unsigned long size,
  	data.set_mask = set_mask;
  	data.clear_mask = clear_mask;
 
-	ret = apply_to_page_range(&init_mm, start, size, change_page_range,
-					&data);
+	ret = walk_page_range_novma(&init_mm, start, start + size,
+				    &pageattr_ops, NULL, &data);
 
  	/*
  	 * If the memory is being made valid without changing any other bits
--

2.30.2

Quoted 4 lines · Lorenzo Stoakes

Quoted 3 lines · Dev Jain

Move away from apply_to_page_range(), which does not honour leaf mappings, to walk_page_range_novma(). The callbacks emit a warning and return EINVAL if a partial range is detected.

Quoted 4 lines

Hm a follow up question here - why not just improve apply_to_page_range() to honour leaf mappings?

What does honouring leaf mappings actually mean? You mean handling huge pages?

Sorry, I always confuse between block, page and leaf mappings :) I mean to say block mappings, yes, huge pages.

Quoted 3 lines

Would it be all that difficult to implement?

That is how I did it initially. But I think we get into the same problem which you are describing w.r.t extending walk_page_range_novma - currently we return EINVAL in case we encounter a block mapping in apply_to_page_range, basically asserting that the callers always operate on page mappings. Removing this assertion and generalizing apply_to_page_range kind of sounds the same as removing the locking assertion and generalizing walk_page_range_novma...

Quoted 5 lines

It seems like you're pushing a bunch of the 'applying' logic over from there to a walker that isn't maybe best suited to it and having to introduce an iffy new form of locking...

IMHO I think it is the reverse. Commit aee16b3cee2746880e40945a9b5bff4f309cfbc4 introduces apply_to_page_range, and commit e6473092bd9116583ce9ab8cf1b6570e1aa6fc83 introduces pagewalk. The commit messages say that the former is meant to be used on page mappings, while the latter is generic. The latter implies that the former was actually never meant to exist...

Quoted 116 lines

Can we go vice-versa? :)

Also obviously walk_page_range_novma() doesn't exist any more :P walk_kernel_page_table_range() is the preferred solution.

Quoted 109 lines

Signed-off-by Dev Jain <[email protected]>

---
  arch/arm64/mm/pageattr.c | 69 +++++++++++++++++++++++++++++++++++++---
  1 file changed, 64 insertions(+), 5 deletions(-)

arch/arm64/mm/pageattr.c +64 -6

index 39fd1f7ff02a..a5c829c64969 100644
@@ -8,6 +8,7 @@
  #include <linux/mem_encrypt.h>
  #include <linux/sched.h>
  #include <linux/vmalloc.h>
+#include <linux/pagewalk.h>
 
  #include <asm/cacheflush.h>
  #include <asm/pgtable-prot.h>

@@ -20,6 +21,67 @@ struct page_change_data {
  	pgprot_t clear_mask;
  };
 
+static pteval_t set_pageattr_masks(unsigned long val, struct mm_walk *walk)
+{
+	struct page_change_data *masks = walk->private;
+	unsigned long new_val = val;
+
+	new_val &= ~(pgprot_val(masks->clear_mask));
+	new_val |= (pgprot_val(masks->set_mask));
+
+	return new_val;
+}
+
+static int pageattr_pud_entry(pud_t *pud, unsigned long addr,
+			      unsigned long next, struct mm_walk *walk)
+{
+	pud_t val = pudp_get(pud);
+
+	if (pud_leaf(val)) {
+		if (WARN_ON_ONCE((next - addr) != PUD_SIZE))
+			return -EINVAL;
+		val = __pud(set_pageattr_masks(pud_val(val), walk));
+		set_pud(pud, val);
+		walk->action = ACTION_CONTINUE;
+	}
+
+	return 0;
+}
+
+static int pageattr_pmd_entry(pmd_t *pmd, unsigned long addr,
+			      unsigned long next, struct mm_walk *walk)
+{
+	pmd_t val = pmdp_get(pmd);
+
+	if (pmd_leaf(val)) {
+		if (WARN_ON_ONCE((next - addr) != PMD_SIZE))
+			return -EINVAL;
+		val = __pmd(set_pageattr_masks(pmd_val(val), walk));
+		set_pmd(pmd, val);
+		walk->action = ACTION_CONTINUE;
+	}
+
+	return 0;
+}
+
+static int pageattr_pte_entry(pte_t *pte, unsigned long addr,
+			      unsigned long next, struct mm_walk *walk)
+{
+	pte_t val = ptep_get(pte);
+
+	val = __pte(set_pageattr_masks(pte_val(val), walk));
+	set_pte(pte, val);
+
+	return 0;
+}
+
+static const struct mm_walk_ops pageattr_ops = {
+	.pud_entry	= pageattr_pud_entry,
+	.pmd_entry	= pageattr_pmd_entry,
+	.pte_entry	= pageattr_pte_entry,
+	.walk_lock	= PGWALK_NOLOCK,
+};
+
  bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED);
 
  bool can_set_direct_map(void)

@@ -49,9 +111,6 @@ static int change_page_range(pte_t *ptep, unsigned long addr, void *data)
  	return 0;
  }
 
-/*
- * This function assumes that the range is mapped with PAGE_SIZE pages.
- */
  static int __change_memory_common(unsigned long start, unsigned long size,
  				pgprot_t set_mask, pgprot_t clear_mask)
  {

@@ -61,8 +120,8 @@ static int __change_memory_common(unsigned long start, unsigned long size,
  	data.set_mask = set_mask;
  	data.clear_mask = clear_mask;
 
-	ret = apply_to_page_range(&init_mm, start, size, change_page_range,
-					&data);
+	ret = walk_page_range_novma(&init_mm, start, start + size,
+				    &pageattr_ops, NULL, &data);
 
  	/*
  	 * If the memory is being made valid without changing any other bits
--

2.30.2

Quoted 11 lines · Dev Jain

Quoted 8 lines · Lorenzo Stoakes

Quoted 3 lines · Dev Jain

Move away from apply_to_page_range(), which does not honour leaf mappings, to walk_page_range_novma(). The callbacks emit a warning and return EINVAL if a partial range is detected.

Hm a follow up question here - why not just improve apply_to_page_range() to honour leaf mappings?

What does honouring leaf mappings actually mean? You mean handling huge pages?

Sorry, I always confuse between block, page and leaf mappings :) I mean to say block mappings, yes, huge pages.

Sometimes I think we like to give different names to things just to make life confusing ;)

Quoted 7 lines

Quoted 1 line

Would it be all that difficult to implement?

That is how I did it initially. But I think we get into the same problem which you are describing w.r.t extending walk_page_range_novma - currently we return EINVAL in case we encounter a block mapping in apply_to_page_range, basically asserting that the callers always operate on page mappings. Removing this assertion and generalizing apply_to_page_range kind of sounds the same as removing the locking assertion and generalizing walk_page_range_novma...

(Again keep in mind walk_page_range_novma no longer exists :)

Yeah it's problematic I guess in that you have a pte_fn_t and would have to get into gross stuff like pretending a PMD entry is a PTE entry etc.

Ugh god why do we do this to ourselves.

Quoted 9 lines

Quoted 4 lines

It seems like you're pushing a bunch of the 'applying' logic over from there to a walker that isn't maybe best suited to it and having to introduce an iffy new form of locking...

IMHO I think it is the reverse. Commit aee16b3cee2746880e40945a9b5bff4f309cfbc4 introduces apply_to_page_range, and commit e6473092bd9116583ce9ab8cf1b6570e1aa6fc83 introduces pagewalk. The commit messages say that the former is meant to be used on page mappings, while the latter is generic. The latter implies that the former was actually never meant to exist...

What a mess.

Maybe the least-worst solution is to just add a new walk_kernel_page_table_range_unlocked() function without an assert and in the comment heavily underline that _you must have made sure this is safe_.

This needs revisting in general, I find the use of init_mm.mmap_lock pretty gross.

Quoted 114 lines

Quoted 114 lines

Can we go vice-versa? :)

Also obviously walk_page_range_novma() doesn't exist any more :P walk_kernel_page_table_range() is the preferred solution.

Quoted 109 lines

Signed-off-by Dev Jain <[email protected]>

---
   arch/arm64/mm/pageattr.c | 69 +++++++++++++++++++++++++++++++++++++---
   1 file changed, 64 insertions(+), 5 deletions(-)

arch/arm64/mm/pageattr.c +64 -6

index 39fd1f7ff02a..a5c829c64969 100644
@@ -8,6 +8,7 @@
   #include <linux/mem_encrypt.h>
   #include <linux/sched.h>
   #include <linux/vmalloc.h>
+#include <linux/pagewalk.h>
 
   #include <asm/cacheflush.h>
   #include <asm/pgtable-prot.h>

@@ -20,6 +21,67 @@ struct page_change_data {
   	pgprot_t clear_mask;
   };
 
+static pteval_t set_pageattr_masks(unsigned long val, struct mm_walk *walk)
+{
+	struct page_change_data *masks = walk->private;
+	unsigned long new_val = val;
+
+	new_val &= ~(pgprot_val(masks->clear_mask));
+	new_val |= (pgprot_val(masks->set_mask));
+
+	return new_val;
+}
+
+static int pageattr_pud_entry(pud_t *pud, unsigned long addr,
+			      unsigned long next, struct mm_walk *walk)
+{
+	pud_t val = pudp_get(pud);
+
+	if (pud_leaf(val)) {
+		if (WARN_ON_ONCE((next - addr) != PUD_SIZE))
+			return -EINVAL;
+		val = __pud(set_pageattr_masks(pud_val(val), walk));
+		set_pud(pud, val);
+		walk->action = ACTION_CONTINUE;
+	}
+
+	return 0;
+}
+
+static int pageattr_pmd_entry(pmd_t *pmd, unsigned long addr,
+			      unsigned long next, struct mm_walk *walk)
+{
+	pmd_t val = pmdp_get(pmd);
+
+	if (pmd_leaf(val)) {
+		if (WARN_ON_ONCE((next - addr) != PMD_SIZE))
+			return -EINVAL;
+		val = __pmd(set_pageattr_masks(pmd_val(val), walk));
+		set_pmd(pmd, val);
+		walk->action = ACTION_CONTINUE;
+	}
+
+	return 0;
+}
+
+static int pageattr_pte_entry(pte_t *pte, unsigned long addr,
+			      unsigned long next, struct mm_walk *walk)
+{
+	pte_t val = ptep_get(pte);
+
+	val = __pte(set_pageattr_masks(pte_val(val), walk));
+	set_pte(pte, val);
+
+	return 0;
+}
+
+static const struct mm_walk_ops pageattr_ops = {
+	.pud_entry	= pageattr_pud_entry,
+	.pmd_entry	= pageattr_pmd_entry,
+	.pte_entry	= pageattr_pte_entry,
+	.walk_lock	= PGWALK_NOLOCK,
+};
+
   bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED);
 
   bool can_set_direct_map(void)

@@ -49,9 +111,6 @@ static int change_page_range(pte_t *ptep, unsigned long addr, void *data)
   	return 0;
   }
 
-/*
- * This function assumes that the range is mapped with PAGE_SIZE pages.
- */
   static int __change_memory_common(unsigned long start, unsigned long size,
   				pgprot_t set_mask, pgprot_t clear_mask)
   {

@@ -61,8 +120,8 @@ static int __change_memory_common(unsigned long start, unsigned long size,
   	data.set_mask = set_mask;
   	data.clear_mask = clear_mask;
 
-	ret = apply_to_page_range(&init_mm, start, size, change_page_range,
-					&data);
+	ret = walk_page_range_novma(&init_mm, start, start + size,
+				    &pageattr_ops, NULL, &data);
 
   	/*
   	 * If the memory is being made valid without changing any other bits
--

2.30.2

Quoted 3 lines · Dev Jain

Move away from apply_to_page_range(), which does not honour leaf mappings, to walk_page_range_novma(). The callbacks emit a warning and return EINVAL if a partial range is detected.

Hm a follow up question here - why not just improve apply_to_page_range() to honour leaf mappings?

What does honouring leaf mappings actually mean? You mean handling huge pages?

Would it be all that difficult to implement?

It seems like you're pushing a bunch of the 'applying' logic over from there to a walker that isn't maybe best suited to it and having to introduce an iffy new form of locking...

Can we go vice-versa? :)

Also obviously walk_page_range_novma() doesn't exist any more :P walk_kernel_page_table_range() is the preferred solution.

Quoted 15 lines · Lorenzo Stoakes

Quoted 3 lines · Dev Jain

Move away from apply_to_page_range(), which does not honour leaf mappings, to walk_page_range_novma(). The callbacks emit a warning and return EINVAL if a partial range is detected.

Hm a follow up question here - why not just improve apply_to_page_range() to honour leaf mappings?

What does honouring leaf mappings actually mean? You mean handling huge pages?

Would it be all that difficult to implement?

It seems like you're pushing a bunch of the 'applying' logic over from there to a walker that isn't maybe best suited to it and having to introduce an iffy new form of locking...

Quoted 4 lines

Can we go vice-versa? :)

Also obviously walk_page_range_novma() doesn't exist any more :P walk_kernel_page_table_range() is the preferred solution.

I cannot see this function in mm-unstable. Also, mm-unstable is broken - I get some RCU message in dmesg, and after some 20-30 second delay the kernel boots. So on which branch do I base my work on...

Quoted 2 lines

Quoted 16 lines · Dev Jain

Quoted 15 lines · Lorenzo Stoakes

Quoted 3 lines · Dev Jain

Move away from apply_to_page_range(), which does not honour leaf mappings, to walk_page_range_novma(). The callbacks emit a warning and return EINVAL if a partial range is detected.

Hm a follow up question here - why not just improve apply_to_page_range() to honour leaf mappings?

What does honouring leaf mappings actually mean? You mean handling huge pages?

Would it be all that difficult to implement?

It seems like you're pushing a bunch of the 'applying' logic over from there to a walker that isn't maybe best suited to it and having to introduce an iffy new form of locking...

Quoted 7 lines

Quoted 4 lines

Can we go vice-versa? :)

Also obviously walk_page_range_novma() doesn't exist any more :P walk_kernel_page_table_range() is the preferred solution.

I cannot see this function in mm-unstable. Also, mm-unstable is broken - I get some RCU message in dmesg, and after some 20-30 second delay the kernel boots. So on which branch do I base my work on...

mm-unstable shouldn't be broken as that's what should be in -next, concerning! Worth investigating... But rebase! :)

Sorry maybe isn't clear as we changed this recently - you should base all changes intended for the next release on mm-new.

As I understand it:

- mm-new is the _rawest_ form of the state of mm, Andrew described it as the 'crazy' branch that basiclaly has everything.

- mm-unstable is when things have been percolating for a while and are considered reasonably stable-ish kinda, but most importantly - ready for -next testing. And is what goes to -next.

- mm-stable is gathered shortly before the merge window starts and is all the stuff Andrew will send to Linus.

To pick up on the most recent changes therefore use mm-new. Using anything else risks issues like this where your patch will conflict, etc.

Another point to note is, during the merge window, mm-new is where changes due for the release after the one being currently merged are kept (e.g. over the past 2 weeks of 6.16 merge window, this would be changes for 6.17).

Not all subsystems even take patches at all during the merge window, but mm does.

So especially during merge window it's important to base you changes on mm-new.

Cheers, Lorenzo

arm64 implements lazy_mmu_mode to allow deferral and batching of barriers when updating kernel PTEs, which provides a nice performance boost. arm64 currently uses apply_to_page_range() to modify kernel PTE permissions, which runs inside lazy_mmu_mode. So to prevent a performance regression, let's add hooks to walk_page_range_novma() to allow continued use of lazy_mmu_mode.

Signed-off-by Dev Jain <[email protected]>

---
Credits to Ryan for the patch description.

 arch/arm64/mm/pageattr.c | 12 ++++++++++++
 include/linux/pagewalk.h |  2 ++
 mm/pagewalk.c            |  6 ++++++
 3 files changed, 20 insertions(+)

arch/arm64/mm/pageattr.c +0 -0

index a5c829c64969..9163324b12a0 100644

arch/arm64/mm/pageattr.c +0 -0

 };
 
 bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED);

include/linux/pagewalk.h +2 -0

index 9bc8853ed3de..2157d345974c 100644
@@ -88,6 +88,8 @@ struct mm_walk_ops {
 	int (*pre_vma)(unsigned long start, unsigned long end,
 		       struct mm_walk *walk);
 	void (*post_vma)(struct mm_walk *walk);
+	void (*pre_pte_table)(void);
+	void (*post_pte_table)(void);
 	int (*install_pte)(unsigned long addr, unsigned long next,
 			   pte_t *ptep, struct mm_walk *walk);
 	enum page_walk_lock walk_lock;

mm/pagewalk.c +0 -0

index 9657cf4664b2..a441f5cbbc45 100644

mm/pagewalk.c +0 -1

 	return err;
 }
 
--

2.30.2

Quoted 21 lines · Dev Jain

arm64 implements lazy_mmu_mode to allow deferral and batching of barriers when updating kernel PTEs, which provides a nice performance boost. arm64 currently uses apply_to_page_range() to modify kernel PTE permissions, which runs inside lazy_mmu_mode. So to prevent a performance regression, let's add hooks to walk_page_range_novma() to allow continued use of lazy_mmu_mode.

Signed-off-by Dev Jain <[email protected]>

---
Credits to Ryan for the patch description.

 arch/arm64/mm/pageattr.c | 12 ++++++++++++
 include/linux/pagewalk.h |  2 ++
 mm/pagewalk.c            |  6 ++++++
 3 files changed, 20 insertions(+)

arch/arm64/mm/pageattr.c +0 -0

index a5c829c64969..9163324b12a0 100644

arch/arm64/mm/pageattr.c static int pageattr_pte_entry(pte_t *pte, unsigned long addr, · line 81

Quoted 7 lines

arch/arm64/mm/pageattr.c +4 -0

@@ -75,11 +75,23 @@ static int pageattr_pte_entry(pte_t *pte, unsigned long addr,
 	return 0;
 }
 
+static void pte_lazy_mmu_enter(void)
+{
+	arch_enter_lazy_mmu_mode();
+}

Hm am I missing something? I don't see this function or the leave version defined in arch/arm64?

No do I see __HAVE_ARCH_ENTER_LAZY_MMU_MODE?

arch/arm64/mm/pageattr.c static int pageattr_pte_entry(pte_t *pte, unsigned long addr, · line 86

Quoted 6 lines

arch/arm64/mm/pageattr.c +5 -0

+
+static void pte_lazy_mmu_leave(void)
+{
+	arch_leave_lazy_mmu_mode();
+}

Are you absolutely sure you will never need to hook this stuff on higher level page tables?

If this relates to vmalloc, then we do have huge page mappings in vmalloc logic?

arch/arm64/mm/pageattr.c static int pageattr_pte_entry(pte_t *pte, unsigned long addr, · line 94

Quoted 9 lines

arch/arm64/mm/pageattr.c +3 -0

+
 static const struct mm_walk_ops pageattr_ops = {
 	.pud_entry	= pageattr_pud_entry,
 	.pmd_entry	= pageattr_pmd_entry,
 	.pte_entry	= pageattr_pte_entry,
 	.walk_lock	= PGWALK_NOLOCK,
+	.pre_pte_table	= pte_lazy_mmu_enter,
+	.post_pte_table	= pte_lazy_mmu_leave,

This is kind of horrid really, are we sure the lazy mmu mode is valid for everything that occurs within the the loop? I suppose it's only simple logic for the ops->pte_entry stuff.

But it feels like hacking something in for this specific case.

At the same time I don't want to get in the way of an optimisation. We could do something in ops->pmd_entry, but then we'd not get to turn it off afterwards...

Same for any higher level page table hm.

Is this really the only way to get this? I guess it's not feasible having this just switched on for the whole operation...

I just fear that we could end up populating these mm_walk_ops with every corner case thing we think of.

Quoted 2 lines

arch/arm64/mm/pageattr.c +0 -0

 };
 

include/linux/pagewalk.h struct mm_walk_ops { · line 92

Quoted 11 lines

arch/arm64/mm/pageattr.c +0 -0

 bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED);

include/linux/pagewalk.h +2 -0

index 9bc8853ed3de..2157d345974c 100644
@@ -88,6 +88,8 @@ struct mm_walk_ops {
 	int (*pre_vma)(unsigned long start, unsigned long end,
 		       struct mm_walk *walk);
 	void (*post_vma)(struct mm_walk *walk);
+	void (*pre_pte_table)(void);
+	void (*post_pte_table)(void);

mm/pagewalk.c static int walk_pte_range_inner(pte_t *pte, unsigned long addr, · line 37

Quoted 13 lines

include/linux/pagewalk.h +0 -0

 	int (*install_pte)(unsigned long addr, unsigned long next,
 			   pte_t *ptep, struct mm_walk *walk);
 	enum page_walk_lock walk_lock;

mm/pagewalk.c +2 -0

index 9657cf4664b2..a441f5cbbc45 100644
@@ -33,6 +33,9 @@ static int walk_pte_range_inner(pte_t *pte, unsigned long addr,
 	const struct mm_walk_ops *ops = walk->ops;
 	int err = 0;
 
+	if (walk->ops->pre_pte_table)
+		walk->ops->pre_pte_table();

NIT: you have 'ops' already, no need for walk-> :)

mm/pagewalk.c static int walk_pte_range_inner(pte_t *pte, unsigned long addr, · line 64

Quoted 11 lines

mm/pagewalk.c +4 -0

+
 	for (;;) {
 		if (ops->install_pte && pte_none(ptep_get(pte))) {
 			pte_t new_pte;

@@ -56,6 +59,9 @@ static int walk_pte_range_inner(pte_t *pte, unsigned long addr,
 		addr += PAGE_SIZE;
 		pte++;
 	}
+
+	if (walk->ops->post_pte_table)
+		walk->ops->post_pte_table();

NIT: same as above.

Quoted 6 lines

mm/pagewalk.c +0 -1

 	return err;
 }
 
--

2.30.2

Quoted 33 lines · Lorenzo Stoakes

Quoted 21 lines · Dev Jain

arm64 implements lazy_mmu_mode to allow deferral and batching of barriers when updating kernel PTEs, which provides a nice performance boost. arm64 currently uses apply_to_page_range() to modify kernel PTE permissions, which runs inside lazy_mmu_mode. So to prevent a performance regression, let's add hooks to walk_page_range_novma() to allow continued use of lazy_mmu_mode.

Signed-off-by Dev Jain <[email protected]>

---
Credits to Ryan for the patch description.

 arch/arm64/mm/pageattr.c | 12 ++++++++++++
 include/linux/pagewalk.h |  2 ++
 mm/pagewalk.c            |  6 ++++++
 3 files changed, 20 insertions(+)

arch/arm64/mm/pageattr.c +0 -0

index a5c829c64969..9163324b12a0 100644

arch/arm64/mm/pageattr.c static int pageattr_pte_entry(pte_t *pte, unsigned long addr, · line 81

Quoted 7 lines

arch/arm64/mm/pageattr.c +4 -0

@@ -75,11 +75,23 @@ static int pageattr_pte_entry(pte_t *pte, unsigned long addr,
 	return 0;
 }
 
+static void pte_lazy_mmu_enter(void)
+{
+	arch_enter_lazy_mmu_mode();
+}

Hm am I missing something? I don't see this function or the leave version defined in arch/arm64?

Quoted 1 line

No do I see __HAVE_ARCH_ENTER_LAZY_MMU_MODE?

arm64 is starting to use lazy_mmu_mode from v6.16 - the changes are in Linus's tree.

Quoted 11 lines

arch/arm64/mm/pageattr.c static int pageattr_pte_entry(pte_t *pte, unsigned long addr, · line 86

Quoted 7 lines

arch/arm64/mm/pageattr.c +5 -0

+
+static void pte_lazy_mmu_leave(void)
+{
+	arch_leave_lazy_mmu_mode();
+}

Are you absolutely sure you will never need to hook this stuff on higher level page tables?

Quoted 1 line

If this relates to vmalloc, then we do have huge page mappings in vmalloc logic?

The performance advantage (for arm64's usage at least) really only (currently) beneficial in practice to PTE level since we can reduce barriers by 512x. And apply_to_page_range() was only using lazy mmu for the pte level anyway.

But actually I think we can do better here...

Quoted 17 lines

arch/arm64/mm/pageattr.c static int pageattr_pte_entry(pte_t *pte, unsigned long addr, · line 94

Quoted 10 lines

arch/arm64/mm/pageattr.c +3 -0

+
 static const struct mm_walk_ops pageattr_ops = {
 	.pud_entry	= pageattr_pud_entry,
 	.pmd_entry	= pageattr_pmd_entry,
 	.pte_entry	= pageattr_pte_entry,
 	.walk_lock	= PGWALK_NOLOCK,
+	.pre_pte_table	= pte_lazy_mmu_enter,
+	.post_pte_table	= pte_lazy_mmu_leave,

This is kind of horrid really, are we sure the lazy mmu mode is valid for everything that occurs within the the loop? I suppose it's only simple logic for the ops->pte_entry stuff.

But it feels like hacking something in for this specific case.

Quoted 7 lines

At the same time I don't want to get in the way of an optimisation. We could do something in ops->pmd_entry, but then we'd not get to turn it off afterwards...

Same for any higher level page table hm.

Is this really the only way to get this? I guess it's not feasible having this just switched on for the whole operation...

...I think you're right. The only reason we traditionally confine the lazy mmu mode to a single page table is because we want to enclose it within the PTL. But that requirement doesn't stand for kernel mappings. As long as the walker can guarrantee that it doesn't allocate any memory (because with certain debug settings that can cause lazy mmu nesting) or try to sleep then I think we can just bracket the whole walk_page_range_novma() call.

So I think we can avoid these new callbacks and just do:

arch_enter_lazy_mmu_mode()
walk_page_range_novma()
arch_leave_lazy_mmu_mode()

That will even give us the benefit of optimizing at PMD/PUD levels.

Quoted 8 lines

I just fear that we could end up populating these mm_walk_ops with every corner case thing we think of.

Quoted 2 lines

arch/arm64/mm/pageattr.c +0 -0

 };
 

Quoted 11 lines

include/linux/pagewalk.h struct mm_walk_ops { · line 92

Quoted 11 lines

arch/arm64/mm/pageattr.c +0 -0

 bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED);

include/linux/pagewalk.h +2 -0

index 9bc8853ed3de..2157d345974c 100644
@@ -88,6 +88,8 @@ struct mm_walk_ops {
 	int (*pre_vma)(unsigned long start, unsigned long end,
 		       struct mm_walk *walk);
 	void (*post_vma)(struct mm_walk *walk);
+	void (*pre_pte_table)(void);
+	void (*post_pte_table)(void);

nit: If we did end up with this approach, I wonder if it's better to generalize and call it pre_table() and post_table(), passing in a level param? In any case, you'll at least want to pass the walk structure.

Quoted 31 lines

Quoted 9 lines

include/linux/pagewalk.h +0 -0

 	int (*install_pte)(unsigned long addr, unsigned long next,
 			   pte_t *ptep, struct mm_walk *walk);
 	enum page_walk_lock walk_lock;

mm/pagewalk.c +0 -0

index 9657cf4664b2..a441f5cbbc45 100644

mm/pagewalk.c static int walk_pte_range_inner(pte_t *pte, unsigned long addr, · line 37

Quoted 5 lines

mm/pagewalk.c +2 -0

@@ -33,6 +33,9 @@ static int walk_pte_range_inner(pte_t *pte, unsigned long addr,
 	const struct mm_walk_ops *ops = walk->ops;
 	int err = 0;
 
+	if (walk->ops->pre_pte_table)
+		walk->ops->pre_pte_table();

NIT: you have 'ops' already, no need for walk-> :)

mm/pagewalk.c static int walk_pte_range_inner(pte_t *pte, unsigned long addr, · line 64

Quoted 11 lines

mm/pagewalk.c +4 -0

+
 	for (;;) {
 		if (ops->install_pte && pte_none(ptep_get(pte))) {
 			pte_t new_pte;

@@ -56,6 +59,9 @@ static int walk_pte_range_inner(pte_t *pte, unsigned long addr,
 		addr += PAGE_SIZE;
 		pte++;
 	}
+
+	if (walk->ops->post_pte_table)
+		walk->ops->post_pte_table();

NIT: same as above.

Quoted 8 lines

Quoted 6 lines

mm/pagewalk.c +0 -1

 	return err;
 }
 
--

2.30.2

This series paves the path to enable huge mappings in vmalloc space by default on arm64. > For this we must ensure that we can handle any permission games on vmalloc space.

Currently, __change_memory_common() uses apply_to_page_range() which does not support changing permissions for leaf mappings.

We attempt to move away from this by using walk_page_range_novma(), similar to what riscv does right now; however, it is the responsibility of the caller to ensure that we do not pass a range, or split the range covering a partial leaf mapping.

This series is tied with Yang Shi's attempt [1] at using huge mappings in the linear mapping in case the system supports BBML2, in which case we will be able to split the linear mapping if needed without break-before-make. Thus, Yang's series, IIUC, will be one such user of my series; suppose we are changing permissions on a range of the linear map backed by PMD-hugepages, then the sequence of operations should look like the following:

split_range(start, (start + HPAGE_PMD_SIZE) & ~HPAGE_PMD_MASK); split_range(end & ~HPAGE_PMD_MASK, end);

__change_memory_common(start, end);

However, this series can be used independently of Yang's; since currently permission games are being played only on pte mappings (due to apply_to_page_range not supporting otherwise), this series provides the mechanism for enabling huge mappings for various kernel mappings like linear map and vmalloc.

[1] https://lore.kernel.org/all/[email protected]/

Dev Jain (3):
mm: Allow pagewalk without locks
arm64: pageattr: Use walk_page_range_novma() to change memory
permissions
mm/pagewalk: Add pre/post_pte_table callback for lazy MMU on arm64

 arch/arm64/mm/pageattr.c | 81 +++++++++++++++++++++++++++++++++++++---
 include/linux/pagewalk.h |  4 ++
 mm/pagewalk.c            | 18 +++++++--
 3 files changed, 94 insertions(+), 9 deletions(-)

Quoted 3 lines · Dev Jain

This series paves the path to enable huge mappings in vmalloc space by default on arm64. > For this we must ensure that we can handle any permission games on vmalloc space.

And the linear map :)

Quoted 3 lines

Currently, __change_memory_common() uses apply_to_page_range() which does not support changing permissions for leaf mappings.

nit: A "leaf mapping" is the lowest level entry in the page tables for a given address - i.e. it maps an address to some actual memory rather than to another pgtable. It includes what the Arm ARM calls "page mappings" (PTE level) and "block mappings" (PMD/PUD/.. level). apply_to_page_range() does support page mappings, so saying it doesn't support leaf mappings is incorrect. It doesn't support block mappings.

Quoted 13 lines

We attempt to move away from this by using walk_page_range_novma(), similar to what riscv does right now; however, it is the responsibility of the caller to ensure that we do not pass a range, or split the range covering a partial leaf mapping.

This series is tied with Yang Shi's attempt [1] at using huge mappings in the linear mapping in case the system supports BBML2, in which case we will be able to split the linear mapping if needed without break-before-make. Thus, Yang's series, IIUC, will be one such user of my series; suppose we are changing permissions on a range of the linear map backed by PMD-hugepages, then the sequence of operations should look like the following:

Quoted 2 lines

split_range(start, (start + HPAGE_PMD_SIZE) & ~HPAGE_PMD_MASK); split_range(end & ~HPAGE_PMD_MASK, end);

I don't understand what the HPAGE_PMD_MASK twiddling is doing? That's not right. It's going to give you the offset within the 2M region. You just want:

split_range(start)
split_range(end)

right?

Quoted 7 lines

__change_memory_common(start, end);

However, this series can be used independently of Yang's; since currently permission games are being played only on pte mappings (due to apply_to_page_range not supporting otherwise), this series provides the mechanism for enabling huge mappings for various kernel mappings like linear map and vmalloc.

In other words, you are saying that this series is a prerequisite for Yang's series (and both are prerequisites for huge vmalloc by default). Your series adds a new capability that Yang's series will rely on (the ability to change permissions on block mappings).

Thanks,
Ryan

Quoted 13 lines

[1] https://lore.kernel.org/all/[email protected]/

Dev Jain (3):
mm: Allow pagewalk without locks
arm64: pageattr: Use walk_page_range_novma() to change memory
permissions
mm/pagewalk: Add pre/post_pte_table callback for lazy MMU on arm64

  arch/arm64/mm/pageattr.c | 81 +++++++++++++++++++++++++++++++++++++---
  include/linux/pagewalk.h |  4 ++
  mm/pagewalk.c            | 18 +++++++--
  3 files changed, 94 insertions(+), 9 deletions(-)

Quoted 9 lines · Ryan Roberts

Quoted 3 lines · Dev Jain

This series paves the path to enable huge mappings in vmalloc space by default on arm64. > For this we must ensure that we can handle any permission games on vmalloc space.

And the linear map :)

Quoted 3 lines

Currently, __change_memory_common() uses apply_to_page_range() which does not support changing permissions for leaf mappings.

Quoted 6 lines

nit: A "leaf mapping" is the lowest level entry in the page tables for a given address - i.e. it maps an address to some actual memory rather than to another pgtable. It includes what the Arm ARM calls "page mappings" (PTE level) and "block mappings" (PMD/PUD/.. level). apply_to_page_range() does support page mappings, so saying it doesn't support leaf mappings is incorrect. It doesn't support block mappings.

Sorry, again got confused by nomenclature : )

Quoted 18 lines

Quoted 13 lines

We attempt to move away from this by using walk_page_range_novma(), similar to what riscv does right now; however, it is the responsibility of the caller to ensure that we do not pass a range, or split the range covering a partial leaf mapping.

This series is tied with Yang Shi's attempt [1] at using huge mappings in the linear mapping in case the system supports BBML2, in which case we will be able to split the linear mapping if needed without break-before-make. Thus, Yang's series, IIUC, will be one such user of my series; suppose we are changing permissions on a range of the linear map backed by PMD-hugepages, then the sequence of operations should look like the following:

Quoted 2 lines

split_range(start, (start + HPAGE_PMD_SIZE) & ~HPAGE_PMD_MASK); split_range(end & ~HPAGE_PMD_MASK, end);

I don't understand what the HPAGE_PMD_MASK twiddling is doing? That's not right. It's going to give you the offset within the 2M region. You just want:

Quoted 4 lines

split_range(start)
split_range(end)

right?

Suppose start = 2M + 4K, end = 8M + 5K. Then my sequence will compute to

split_range(2M + 4K, 3M)
split_range(8M, 8M + 5K)

__change_memory_common(2M + 4K, 8M + 5K)

So now __change_memory_common() wouldn't have to deal with splitting the starts and ends. Please correct me if I am wrong.

Quoted 8 lines

Quoted 8 lines

__change_memory_common(start, end);

However, this series can be used independently of Yang's; since currently permission games are being played only on pte mappings (due to apply_to_page_range
not supporting otherwise), this series provides the mechanism for enabling huge mappings for various kernel mappings like linear map and vmalloc.

Quoted 4 lines

In other words, you are saying that this series is a prerequisite for Yang's series (and both are prerequisites for huge vmalloc by default). Your series adds a new capability that Yang's series will rely on (the ability to change permissions on block mappings).

That's right.

Quoted 18 lines

Thanks,
Ryan

Quoted 14 lines

[1] https://lore.kernel.org/all/20250304222018.615808-1-
[email protected]/

Dev Jain (3):
   mm: Allow pagewalk without locks
   arm64: pageattr: Use walk_page_range_novma() to change memory permissions
   mm/pagewalk: Add pre/post_pte_table callback for lazy MMU on arm64

  arch/arm64/mm/pageattr.c | 81 +++++++++++++++++++++++++++++++++++++--- include/linux/pagewalk.h |  4 ++
  mm/pagewalk.c            | 18 +++++++--

  3 files changed, 94 insertions(+), 9 deletions(-)

Quoted 35 lines · Dev Jain

Quoted 9 lines · Ryan Roberts

Quoted 3 lines · Dev Jain

This series paves the path to enable huge mappings in vmalloc space by default on arm64. > For this we must ensure that we can handle any permission games on vmalloc space.

And the linear map :)

Quoted 3 lines

Currently, __change_memory_common() uses apply_to_page_range() which does not support changing permissions for leaf mappings.

Quoted 6 lines

nit: A "leaf mapping" is the lowest level entry in the page tables for a given address - i.e. it maps an address to some actual memory rather than to another pgtable. It includes what the Arm ARM calls "page mappings" (PTE level) and "block mappings" (PMD/PUD/.. level). apply_to_page_range() does support page mappings, so saying it doesn't support leaf mappings is incorrect. It doesn't support block mappings.

Sorry, again got confused by nomenclature : )

Quoted 17 lines

Quoted 12 lines

We attempt to move away from this by using walk_page_range_novma(), similar to what riscv does right now; however, it is the responsibility of the caller to ensure that we do not pass a range, or split the range covering a partial leaf mapping.

This series is tied with Yang Shi's attempt [1] at using huge mappings in the linear mapping in case the system supports BBML2, in which case we will be able to split the linear mapping if needed without break-before-make. Thus, Yang's series, IIUC, will be one such user of my series; suppose we are changing permissions on a range of the linear map backed by PMD-hugepages, then the sequence of operations should look like the following:

Quoted 2 lines

split_range(start, (start + HPAGE_PMD_SIZE) & ~HPAGE_PMD_MASK); split_range(end & ~HPAGE_PMD_MASK, end);

I don't understand what the HPAGE_PMD_MASK twiddling is doing? That's not right. It's going to give you the offset within the 2M region. You just want:

Quoted 5 lines

Quoted 4 lines

split_range(start)
split_range(end)

right?

Suppose start = 2M + 4K, end = 8M + 5K. Then my sequence will compute to

8M + 5K is not a valid split point. It has to be at least page aligned.

Quoted 2 lines

split_range(2M + 4K, 3M)
split_range(8M, 8M + 5K)

We just want to split at start and end. What are the 3M and 8M params supposed to be? Anyway, this is off-topic for this series.

Quoted 36 lines

__change_memory_common(2M + 4K, 8M + 5K)

So now __change_memory_common() wouldn't have to deal with splitting the starts and ends. Please correct me if I am wrong.

Quoted 11 lines

Quoted 7 lines

__change_memory_common(start, end);

However, this series can be used independently of Yang's; since currently permission games are being played only on pte mappings (due to apply_to_page_range
not supporting otherwise), this series provides the mechanism for enabling huge mappings for various kernel mappings like linear map and vmalloc.

In other words, you are saying that this series is a prerequisite for Yang's series (and both are prerequisites for huge vmalloc by default). Your series adds a new capability that Yang's series will rely on (the ability to change permissions on block mappings).

That's right.

Quoted 17 lines

Thanks,
Ryan

Quoted 14 lines

[1] https://lore.kernel.org/all/20250304222018.615808-1-
[email protected]/

Dev Jain (3):
   mm: Allow pagewalk without locks
   arm64: pageattr: Use walk_page_range_novma() to change memory permissions
   mm/pagewalk: Add pre/post_pte_table callback for lazy MMU on arm64

   arch/arm64/mm/pageattr.c | 81 +++++++++++++++++++++++++++++++++++++---
   include/linux/pagewalk.h |  4 ++
   mm/pagewalk.c            | 18 +++++++--
   3 files changed, 94 insertions(+), 9 deletions(-)

Quoted 38 lines · Ryan Roberts

Quoted 37 lines · Dev Jain

Quoted 9 lines · Ryan Roberts

Quoted 3 lines · Dev Jain

This series paves the path to enable huge mappings in vmalloc space by default on arm64. > For this we must ensure that we can handle any permission games on vmalloc space.

And the linear map :)

Quoted 3 lines

Currently, __change_memory_common() uses apply_to_page_range() which does not support changing permissions for leaf mappings.

Quoted 6 lines

nit: A "leaf mapping" is the lowest level entry in the page tables for a given address - i.e. it maps an address to some actual memory rather than to another pgtable. It includes what the Arm ARM calls "page mappings" (PTE level) and "block mappings" (PMD/PUD/.. level). apply_to_page_range() does support page mappings, so saying it doesn't support leaf mappings is incorrect. It doesn't support block mappings.

Sorry, again got confused by nomenclature : )

Quoted 19 lines

Quoted 13 lines

We attempt to move away from this by using walk_page_range_novma(), similar to what riscv does right now; however, it is the responsibility of the caller to ensure that we do not pass a range, or split the range covering a partial leaf mapping.

This series is tied with Yang Shi's attempt [1] at using huge mappings in the linear mapping in case the system supports BBML2, in which case we will be able to split the linear mapping if needed without break-before- make.
Thus, Yang's series, IIUC, will be one such user of my series; suppose we are changing permissions on a range of the linear map backed by PMD-hugepages, then the sequence of operations should look like the following:

Quoted 2 lines

split_range(start, (start + HPAGE_PMD_SIZE) & ~HPAGE_PMD_MASK); split_range(end & ~HPAGE_PMD_MASK, end);

I don't understand what the HPAGE_PMD_MASK twiddling is doing? That's not right.
It's going to give you the offset within the 2M region. You just want:

Quoted 6 lines

Quoted 5 lines

Quoted 4 lines

split_range(start)
split_range(end)

right?

Suppose start = 2M + 4K, end = 8M + 5K. Then my sequence will compute to

8M + 5K is not a valid split point. It has to be at least page aligned.

Sorry, so consider 8M + 4K.

Quoted 2 lines

Quoted 2 lines

split_range(2M + 4K, 3M)
split_range(8M, 8M + 5K)

Quoted 2 lines

We just want to split at start and end. What are the 3M and 8M params supposed to be? Anyway, this is off-topic for this series.

I think we are both saying the same thing; yes we will split only the start and the end,
so if the address 2Mb + 4Kb is mapped as a PMD-hugepage, we need to split this PMD into
a PTE table, which will map 2Mb till 4Mb as base pages now.

Quoted 37 lines

Quoted 37 lines

__change_memory_common(2M + 4K, 8M + 5K)

So now __change_memory_common() wouldn't have to deal with splitting the starts and ends. Please correct me if I am wrong.

Quoted 11 lines

Quoted 7 lines

__change_memory_common(start, end);

However, this series can be used independently of Yang's; since currently permission games are being played only on pte mappings (due to apply_to_page_range
not supporting otherwise), this series provides the mechanism for enabling huge mappings for various kernel mappings like linear map and vmalloc.

In other words, you are saying that this series is a prerequisite for Yang's series (and both are prerequisites for huge vmalloc by default). Your series adds a new capability that Yang's series will rely on (the ability to change permissions on block mappings).

That's right.

Quoted 17 lines

Thanks,
Ryan

Quoted 14 lines

[1] https://lore.kernel.org/all/20250304222018.615808-1-
[email protected]/

Dev Jain (3):
    mm: Allow pagewalk without locks
    arm64: pageattr: Use walk_page_range_novma() to change memory permissions
    mm/pagewalk: Add pre/post_pte_table callback for lazy MMU on arm64

   arch/arm64/mm/pageattr.c | 81 +++++++++++++++++++++++++++++++++++++--- include/linux/pagewalk.h |  4 ++
   mm/pagewalk.c            | 18 +++++++--

   3 files changed, 94 insertions(+), 9 deletions(-)

Quoted 49 lines · Dev Jain

Quoted 38 lines · Ryan Roberts

Quoted 37 lines · Dev Jain

Quoted 9 lines · Ryan Roberts

Quoted 3 lines · Dev Jain

This series paves the path to enable huge mappings in vmalloc space by default on arm64. > For this we must ensure that we can handle any permission games on vmalloc space.

And the linear map :)

Quoted 3 lines

Currently, __change_memory_common() uses apply_to_page_range() which does not support changing permissions for leaf mappings.

Quoted 6 lines

nit: A "leaf mapping" is the lowest level entry in the page tables for a given address - i.e. it maps an address to some actual memory rather than to another pgtable. It includes what the Arm ARM calls "page mappings" (PTE level) and "block mappings" (PMD/PUD/.. level). apply_to_page_range() does support page mappings, so saying it doesn't support leaf mappings is incorrect. It doesn't support block mappings.

Sorry, again got confused by nomenclature : )

Quoted 19 lines

Quoted 13 lines

We attempt to move away from this by using walk_page_range_novma(), similar to what riscv does right now; however, it is the responsibility of the caller to ensure that we do not pass a range, or split the range covering a partial leaf mapping.

This series is tied with Yang Shi's attempt [1] at using huge mappings in the linear mapping in case the system supports BBML2, in which case we will be able to split the linear mapping if needed without break-before- make.
Thus, Yang's series, IIUC, will be one such user of my series; suppose we are changing permissions on a range of the linear map backed by PMD-hugepages, then the sequence of operations should look like the following:

Quoted 2 lines

split_range(start, (start + HPAGE_PMD_SIZE) & ~HPAGE_PMD_MASK); split_range(end & ~HPAGE_PMD_MASK, end);

I don't understand what the HPAGE_PMD_MASK twiddling is doing? That's not right.
It's going to give you the offset within the 2M region. You just want:

Quoted 6 lines

Quoted 5 lines

Quoted 4 lines

split_range(start)
split_range(end)

right?

Suppose start = 2M + 4K, end = 8M + 5K. Then my sequence will compute to

8M + 5K is not a valid split point. It has to be at least page aligned.

Sorry, so consider 8M + 4K.

Quoted 2 lines

Quoted 2 lines

split_range(2M + 4K, 3M)
split_range(8M, 8M + 5K)

Quoted 7 lines

Quoted 2 lines

We just want to split at start and end. What are the 3M and 8M params supposed to be? Anyway, this is off-topic for this series.

I think we are both saying the same thing; yes we will split only the start and the end,
so if the address 2Mb + 4Kb is mapped as a PMD-hugepage, we need to split this PMD into
a PTE table, which will map 2Mb till 4Mb as base pages now.

Not quite; if you start with [2M, 4M) mapped by PMD, then want to ensure a boundary at 2M+4K, I think you would end up with the first 64K (16 pages) mapped by base page, then the then the next 1984K mapped by contpte blocks (31 contpte blocks).

But yes, we agree :)

Quoted 36 lines

Quoted 36 lines

Quoted 36 lines

__change_memory_common(2M + 4K, 8M + 5K)

So now __change_memory_common() wouldn't have to deal with splitting the starts and ends. Please correct me if I am wrong.

Quoted 11 lines

Quoted 7 lines

__change_memory_common(start, end);

However, this series can be used independently of Yang's; since currently permission games are being played only on pte mappings (due to apply_to_page_range
not supporting otherwise), this series provides the mechanism for enabling huge mappings for various kernel mappings like linear map and vmalloc.

In other words, you are saying that this series is a prerequisite for Yang's series (and both are prerequisites for huge vmalloc by default). Your series adds a new capability that Yang's series will rely on (the ability to change permissions on block mappings).

That's right.

Quoted 17 lines

Thanks,
Ryan

Quoted 14 lines

[1] https://lore.kernel.org/all/20250304222018.615808-1-
[email protected]/

Dev Jain (3):
    mm: Allow pagewalk without locks
    arm64: pageattr: Use walk_page_range_novma() to change memory permissions
    mm/pagewalk: Add pre/post_pte_table callback for lazy MMU on arm64

    arch/arm64/mm/pageattr.c | 81 +++++++++++++++++++++++++++++++++++++---
    include/linux/pagewalk.h |  4 ++
    mm/pagewalk.c            | 18 +++++++--
    3 files changed, 94 insertions(+), 9 deletions(-)