[PATCH v3 0/5] move per-vma lock into vm_area_struct

[PATCH v3 0/5] move per-vma lock into vm_area_struct

[Suren Baghdasaryan]

Back when per-vma locks were introduces, vm_lock was moved out of
vm_area_struct in [1] because of the performance regression caused by
false cacheline sharing. Recent investigation [2] revealed that the
regressions is limited to a rather old Broadwell microarchitecture and
even there it can be mitigated by disabling adjacent cacheline
prefetching, see [3].
Splitting single logical structure into multiple ones leads to more
complicated management, extra pointer dereferences and overall less
maintainable code. When that split-away part is a lock, it complicates
things even further. With no performance benefits, there are no reasons
for this split. Merging the vm_lock back into vm_area_struct also allows
vm_area_struct to use SLAB_TYPESAFE_BY_RCU later in this patchset.
This patchset:
1. moves vm_lock back into vm_area_struct, aligning it at the cacheline
boundary and changing the cache to be cacheline-aligned to minimize
cacheline sharing;
2. changes vm_area_struct initialization to mark new vma as detached until
it is inserted into vma tree;
3. changes vm_area_struct cache to SLAB_TYPESAFE_BY_RCU to allow for their
reuse and to minimize call_rcu() calls.
Pagefault microbenchmarks do not show noticeable performance change.

Changes since v2 [4]
- Updated vma_start_read_locked and vma_start_read_locked_nested comments,
per Lorenzo Stoakes;
- Undated patch 2 description to include motivation for moving vm_lock
into vm_area_struct, per Lorenzo Stoakes, Vlastimil Babka, Mateusz Guzik;
- Undated description to include a note about memory overhead dependency
on kernel config options, per Lorenzo Stoakes and Vlastimil Babka;
- Updated vma tests;
- Introduced vma_mark_attached();
- Moved vma_mark_attached() calls into vma_iter_store{_gfp},
per Lorenzo Stoakes
- Moved vma_lock_init() into vm_area_cachep constructor, per Hugh Dickins;
- Ensure vm_lock is preserved when clearing or copying vm_area_struct;
- Pass mm to vma_start_read() to avoid use of vma->vm_mm, which might be
changed by a racing vma_init() if the vma gets reused;
- Added vma write-locking in vma_mark_attached() to prevent races with
vma->detached check in lock_vma_under_rcu().

Patch applies over mm-unstable

[1] https://lore.kernel.org/all/20230227173632.3292573-34-surenb@google.com/
[2] https://lore.kernel.org/all/ZsQyI%2F087V34JoIt@xsang-OptiPlex-9020/
[3] https://lore.kernel.org/all/CAJuCfpEisU8Lfe96AYJDZ+OM4NoPmnw9bP53cT_kbfP_pR+-2g@mail.gmail.com/
[4] https://lore.kernel.org/all/20241112194635.444146-1-surenb@google.com/

Suren Baghdasaryan (5):
  mm: introduce vma_start_read_locked{_nested} helpers
  mm: move per-vma lock into vm_area_struct
  mm: mark vma as detached until it's added into vma tree
  mm: make vma cache SLAB_TYPESAFE_BY_RCU
  docs/mm: document latest changes to vm_lock

 Documentation/mm/process_addrs.rst |  10 ++-
 include/linux/mm.h                 | 121 +++++++++++++++++++++++------
 include/linux/mm_types.h           |  19 ++---
 kernel/fork.c                      |  88 ++++++++-------------
 mm/memory.c                        |   9 ++-
 mm/userfaultfd.c                   |  22 ++----
 mm/vma.c                           |   8 +-
 mm/vma.h                           |   2 +
 tools/testing/vma/vma_internal.h   |  55 ++++++-------
 9 files changed, 183 insertions(+), 151 deletions(-)


base-commit: 84f7f75fda11c147979984755011c6e2b71e97e1
-- 
2.47.0.338.g60cca15819-goog

[PATCH v3 1/5] mm: introduce vma_start_read_locked{_nested} helpers

[Suren Baghdasaryan]

Introduce helper functions which can be used to read-lock a VMA when
holding mmap_lock for read. Replace direct accesses to vma->vm_lock
with these new helpers.

Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
 include/linux/mm.h | 24 ++++++++++++++++++++++++
 mm/userfaultfd.c   | 22 +++++-----------------
 2 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index fecd47239fa9..1ba2e480ae63 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -722,6 +722,30 @@ static inline bool vma_start_read(struct vm_area_struct *vma)
 	return true;
 }
 
+/*
+ * Use only while holding mmap read lock which guarantees that locking will not
+ * fail (nobody can concurrently write-lock the vma). vma_start_read() should
+ * not be used in such cases because it might fail due to mm_lock_seq overflow.
+ * This functionality is used to obtain vma read lock and drop the mmap read lock.
+ */
+static inline void vma_start_read_locked_nested(struct vm_area_struct *vma, int subclass)
+{
+	mmap_assert_locked(vma->vm_mm);
+	down_read_nested(&vma->vm_lock->lock, subclass);
+}
+
+/*
+ * Use only while holding mmap read lock which guarantees that locking will not
+ * fail (nobody can concurrently write-lock the vma). vma_start_read() should
+ * not be used in such cases because it might fail due to mm_lock_seq overflow.
+ * This functionality is used to obtain vma read lock and drop the mmap read lock.
+ */
+static inline void vma_start_read_locked(struct vm_area_struct *vma)
+{
+	mmap_assert_locked(vma->vm_mm);
+	down_read(&vma->vm_lock->lock);
+}
+
 static inline void vma_end_read(struct vm_area_struct *vma)
 {
 	rcu_read_lock(); /* keeps vma alive till the end of up_read */
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index 60a0be33766f..87db4b32b82a 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -84,16 +84,8 @@ static struct vm_area_struct *uffd_lock_vma(struct mm_struct *mm,
 
 	mmap_read_lock(mm);
 	vma = find_vma_and_prepare_anon(mm, address);
-	if (!IS_ERR(vma)) {
-		/*
-		 * We cannot use vma_start_read() as it may fail due to
-		 * false locked (see comment in vma_start_read()). We
-		 * can avoid that by directly locking vm_lock under
-		 * mmap_lock, which guarantees that nobody can lock the
-		 * vma for write (vma_start_write()) under us.
-		 */
-		down_read(&vma->vm_lock->lock);
-	}
+	if (!IS_ERR(vma))
+		vma_start_read_locked(vma);
 
 	mmap_read_unlock(mm);
 	return vma;
@@ -1476,14 +1468,10 @@ static int uffd_move_lock(struct mm_struct *mm,
 	mmap_read_lock(mm);
 	err = find_vmas_mm_locked(mm, dst_start, src_start, dst_vmap, src_vmap);
 	if (!err) {
-		/*
-		 * See comment in uffd_lock_vma() as to why not using
-		 * vma_start_read() here.
-		 */
-		down_read(&(*dst_vmap)->vm_lock->lock);
+		vma_start_read_locked(*dst_vmap);
 		if (*dst_vmap != *src_vmap)
-			down_read_nested(&(*src_vmap)->vm_lock->lock,
-					 SINGLE_DEPTH_NESTING);
+			vma_start_read_locked_nested(*src_vmap,
+						SINGLE_DEPTH_NESTING);
 	}
 	mmap_read_unlock(mm);
 	return err;
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v3 1/5] mm: introduce vma_start_read_locked{_nested} helpers

[Lorenzo Stoakes]

On Sun, Nov 17, 2024 at 12:09:27AM -0800, Suren Baghdasaryan wrote:
> Introduce helper functions which can be used to read-lock a VMA when
> holding mmap_lock for read. Replace direct accesses to vma->vm_lock
> with these new helpers.
>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>

LGTM thanks:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

> ---
>  include/linux/mm.h | 24 ++++++++++++++++++++++++
>  mm/userfaultfd.c   | 22 +++++-----------------
>  2 files changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index fecd47239fa9..1ba2e480ae63 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -722,6 +722,30 @@ static inline bool vma_start_read(struct vm_area_struct *vma)
>  	return true;
>  }
>
> +/*
> + * Use only while holding mmap read lock which guarantees that locking will not
> + * fail (nobody can concurrently write-lock the vma). vma_start_read() should
> + * not be used in such cases because it might fail due to mm_lock_seq overflow.
> + * This functionality is used to obtain vma read lock and drop the mmap read lock.
> + */
> +static inline void vma_start_read_locked_nested(struct vm_area_struct *vma, int subclass)
> +{
> +	mmap_assert_locked(vma->vm_mm);
> +	down_read_nested(&vma->vm_lock->lock, subclass);
> +}
> +
> +/*
> + * Use only while holding mmap read lock which guarantees that locking will not
> + * fail (nobody can concurrently write-lock the vma). vma_start_read() should
> + * not be used in such cases because it might fail due to mm_lock_seq overflow.
> + * This functionality is used to obtain vma read lock and drop the mmap read lock.
> + */
> +static inline void vma_start_read_locked(struct vm_area_struct *vma)
> +{
> +	mmap_assert_locked(vma->vm_mm);
> +	down_read(&vma->vm_lock->lock);
> +}
> +
>  static inline void vma_end_read(struct vm_area_struct *vma)
>  {
>  	rcu_read_lock(); /* keeps vma alive till the end of up_read */
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index 60a0be33766f..87db4b32b82a 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -84,16 +84,8 @@ static struct vm_area_struct *uffd_lock_vma(struct mm_struct *mm,
>
>  	mmap_read_lock(mm);
>  	vma = find_vma_and_prepare_anon(mm, address);
> -	if (!IS_ERR(vma)) {
> -		/*
> -		 * We cannot use vma_start_read() as it may fail due to
> -		 * false locked (see comment in vma_start_read()). We
> -		 * can avoid that by directly locking vm_lock under
> -		 * mmap_lock, which guarantees that nobody can lock the
> -		 * vma for write (vma_start_write()) under us.
> -		 */
> -		down_read(&vma->vm_lock->lock);
> -	}
> +	if (!IS_ERR(vma))
> +		vma_start_read_locked(vma);
>
>  	mmap_read_unlock(mm);
>  	return vma;
> @@ -1476,14 +1468,10 @@ static int uffd_move_lock(struct mm_struct *mm,
>  	mmap_read_lock(mm);
>  	err = find_vmas_mm_locked(mm, dst_start, src_start, dst_vmap, src_vmap);
>  	if (!err) {
> -		/*
> -		 * See comment in uffd_lock_vma() as to why not using
> -		 * vma_start_read() here.
> -		 */
> -		down_read(&(*dst_vmap)->vm_lock->lock);
> +		vma_start_read_locked(*dst_vmap);
>  		if (*dst_vmap != *src_vmap)
> -			down_read_nested(&(*src_vmap)->vm_lock->lock,
> -					 SINGLE_DEPTH_NESTING);
> +			vma_start_read_locked_nested(*src_vmap,
> +						SINGLE_DEPTH_NESTING);
>  	}
>  	mmap_read_unlock(mm);
>  	return err;
> --
> 2.47.0.338.g60cca15819-goog
>

Re: [PATCH v3 1/5] mm: introduce vma_start_read_locked{_nested} helpers

[Davidlohr Bueso]

On Sun, 17 Nov 2024, Suren Baghdasaryan wrote:

>Introduce helper functions which can be used to read-lock a VMA when
>holding mmap_lock for read. Replace direct accesses to vma->vm_lock
>with these new helpers.
>

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

>Signed-off-by: Suren Baghdasaryan <surenb@google.com>

[PATCH v3 2/5] mm: move per-vma lock into vm_area_struct

[Suren Baghdasaryan]

Back when per-vma locks were introduces, vm_lock was moved out of
vm_area_struct in [1] because of the performance regression caused by
false cacheline sharing. Recent investigation [2] revealed that the
regressions is limited to a rather old Broadwell microarchitecture and
even there it can be mitigated by disabling adjacent cacheline
prefetching, see [3].
Splitting single logical structure into multiple ones leads to more
complicated management, extra pointer dereferences and overall less
maintainable code. When that split-away part is a lock, it complicates
things even further. With no performance benefits, there are no reasons
for this split. Merging the vm_lock back into vm_area_struct also allows
vm_area_struct to use SLAB_TYPESAFE_BY_RCU later in this patchset.
Move vm_lock back into vm_area_struct, aligning it at the cacheline
boundary and changing the cache to be cacheline-aligned as well.
With kernel compiled using defconfig, this causes VMA memory consumption
to grow from 160 (vm_area_struct) + 40 (vm_lock) bytes to 256 bytes:

    slabinfo before:
     <name>           ... <objsize> <objperslab> <pagesperslab> : ...
     vma_lock         ...     40  102    1 : ...
     vm_area_struct   ...    160   51    2 : ...

    slabinfo after moving vm_lock:
     <name>           ... <objsize> <objperslab> <pagesperslab> : ...
     vm_area_struct   ...    256   32    2 : ...

Aggregate VMA memory consumption per 1000 VMAs grows from 50 to 64 pages,
which is 5.5MB per 100000 VMAs. Note that the size of this structure is
dependent on the kernel configuration and typically the original size is
higher than 160 bytes. Therefore these calculations are close to the
worst case scenario. A more realistic vm_area_struct usage before this
change is:

     <name>           ... <objsize> <objperslab> <pagesperslab> : ...
     vma_lock         ...     40  102    1 : ...
     vm_area_struct   ...    176   46    2 : ...

Aggregate VMA memory consumption per 1000 VMAs grows from 54 to 64 pages,
which is 3.9MB per 100000 VMAs.
This memory consumption growth can be addressed later by optimizing the
vm_lock.

[1] https://lore.kernel.org/all/20230227173632.3292573-34-surenb@google.com/
[2] https://lore.kernel.org/all/ZsQyI%2F087V34JoIt@xsang-OptiPlex-9020/
[3] https://lore.kernel.org/all/CAJuCfpEisU8Lfe96AYJDZ+OM4NoPmnw9bP53cT_kbfP_pR+-2g@mail.gmail.com/

Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
 include/linux/mm.h               | 28 ++++++++++--------
 include/linux/mm_types.h         |  6 ++--
 kernel/fork.c                    | 49 ++++----------------------------
 tools/testing/vma/vma_internal.h | 33 +++++----------------
 4 files changed, 32 insertions(+), 84 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 1ba2e480ae63..737c003b0a1e 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -684,6 +684,12 @@ static inline void vma_numab_state_free(struct vm_area_struct *vma) {}
 #endif /* CONFIG_NUMA_BALANCING */
 
 #ifdef CONFIG_PER_VMA_LOCK
+static inline void vma_lock_init(struct vm_area_struct *vma)
+{
+	init_rwsem(&vma->vm_lock.lock);
+	vma->vm_lock_seq = UINT_MAX;
+}
+
 /*
  * Try to read-lock a vma. The function is allowed to occasionally yield false
  * locked result to avoid performance overhead, in which case we fall back to
@@ -701,7 +707,7 @@ static inline bool vma_start_read(struct vm_area_struct *vma)
 	if (READ_ONCE(vma->vm_lock_seq) == READ_ONCE(vma->vm_mm->mm_lock_seq.sequence))
 		return false;
 
-	if (unlikely(down_read_trylock(&vma->vm_lock->lock) == 0))
+	if (unlikely(down_read_trylock(&vma->vm_lock.lock) == 0))
 		return false;
 
 	/*
@@ -716,7 +722,7 @@ static inline bool vma_start_read(struct vm_area_struct *vma)
 	 * This pairs with RELEASE semantics in vma_end_write_all().
 	 */
 	if (unlikely(vma->vm_lock_seq == raw_read_seqcount(&vma->vm_mm->mm_lock_seq))) {
-		up_read(&vma->vm_lock->lock);
+		up_read(&vma->vm_lock.lock);
 		return false;
 	}
 	return true;
@@ -731,7 +737,7 @@ static inline bool vma_start_read(struct vm_area_struct *vma)
 static inline void vma_start_read_locked_nested(struct vm_area_struct *vma, int subclass)
 {
 	mmap_assert_locked(vma->vm_mm);
-	down_read_nested(&vma->vm_lock->lock, subclass);
+	down_read_nested(&vma->vm_lock.lock, subclass);
 }
 
 /*
@@ -743,13 +749,13 @@ static inline void vma_start_read_locked_nested(struct vm_area_struct *vma, int
 static inline void vma_start_read_locked(struct vm_area_struct *vma)
 {
 	mmap_assert_locked(vma->vm_mm);
-	down_read(&vma->vm_lock->lock);
+	down_read(&vma->vm_lock.lock);
 }
 
 static inline void vma_end_read(struct vm_area_struct *vma)
 {
 	rcu_read_lock(); /* keeps vma alive till the end of up_read */
-	up_read(&vma->vm_lock->lock);
+	up_read(&vma->vm_lock.lock);
 	rcu_read_unlock();
 }
 
@@ -778,7 +784,7 @@ static inline void vma_start_write(struct vm_area_struct *vma)
 	if (__is_vma_write_locked(vma, &mm_lock_seq))
 		return;
 
-	down_write(&vma->vm_lock->lock);
+	down_write(&vma->vm_lock.lock);
 	/*
 	 * We should use WRITE_ONCE() here because we can have concurrent reads
 	 * from the early lockless pessimistic check in vma_start_read().
@@ -786,7 +792,7 @@ static inline void vma_start_write(struct vm_area_struct *vma)
 	 * we should use WRITE_ONCE() for cleanliness and to keep KCSAN happy.
 	 */
 	WRITE_ONCE(vma->vm_lock_seq, mm_lock_seq);
-	up_write(&vma->vm_lock->lock);
+	up_write(&vma->vm_lock.lock);
 }
 
 static inline void vma_assert_write_locked(struct vm_area_struct *vma)
@@ -798,7 +804,7 @@ static inline void vma_assert_write_locked(struct vm_area_struct *vma)
 
 static inline void vma_assert_locked(struct vm_area_struct *vma)
 {
-	if (!rwsem_is_locked(&vma->vm_lock->lock))
+	if (!rwsem_is_locked(&vma->vm_lock.lock))
 		vma_assert_write_locked(vma);
 }
 
@@ -831,6 +837,7 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
 
 #else /* CONFIG_PER_VMA_LOCK */
 
+static inline void vma_lock_init(struct vm_area_struct *vma) {}
 static inline bool vma_start_read(struct vm_area_struct *vma)
 		{ return false; }
 static inline void vma_end_read(struct vm_area_struct *vma) {}
@@ -865,10 +872,6 @@ static inline void assert_fault_locked(struct vm_fault *vmf)
 
 extern const struct vm_operations_struct vma_dummy_vm_ops;
 
-/*
- * WARNING: vma_init does not initialize vma->vm_lock.
- * Use vm_area_alloc()/vm_area_free() if vma needs locking.
- */
 static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
 {
 	memset(vma, 0, sizeof(*vma));
@@ -877,6 +880,7 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
 	INIT_LIST_HEAD(&vma->anon_vma_chain);
 	vma_mark_detached(vma, false);
 	vma_numab_state_init(vma);
+	vma_lock_init(vma);
 }
 
 /* Use when VMA is not part of the VMA tree and needs no locking */
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 80fef38d9d64..5c4bfdcfac72 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -716,8 +716,6 @@ struct vm_area_struct {
 	 * slowpath.
 	 */
 	unsigned int vm_lock_seq;
-	/* Unstable RCU readers are allowed to read this. */
-	struct vma_lock *vm_lock;
 #endif
 
 	/*
@@ -770,6 +768,10 @@ struct vm_area_struct {
 	struct vma_numab_state *numab_state;	/* NUMA Balancing state */
 #endif
 	struct vm_userfaultfd_ctx vm_userfaultfd_ctx;
+#ifdef CONFIG_PER_VMA_LOCK
+	/* Unstable RCU readers are allowed to read this. */
+	struct vma_lock vm_lock ____cacheline_aligned_in_smp;
+#endif
 } __randomize_layout;
 
 #ifdef CONFIG_NUMA
diff --git a/kernel/fork.c b/kernel/fork.c
index 0061cf2450ef..7823797e31d2 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -436,35 +436,6 @@ static struct kmem_cache *vm_area_cachep;
 /* SLAB cache for mm_struct structures (tsk->mm) */
 static struct kmem_cache *mm_cachep;
 
-#ifdef CONFIG_PER_VMA_LOCK
-
-/* SLAB cache for vm_area_struct.lock */
-static struct kmem_cache *vma_lock_cachep;
-
-static bool vma_lock_alloc(struct vm_area_struct *vma)
-{
-	vma->vm_lock = kmem_cache_alloc(vma_lock_cachep, GFP_KERNEL);
-	if (!vma->vm_lock)
-		return false;
-
-	init_rwsem(&vma->vm_lock->lock);
-	vma->vm_lock_seq = UINT_MAX;
-
-	return true;
-}
-
-static inline void vma_lock_free(struct vm_area_struct *vma)
-{
-	kmem_cache_free(vma_lock_cachep, vma->vm_lock);
-}
-
-#else /* CONFIG_PER_VMA_LOCK */
-
-static inline bool vma_lock_alloc(struct vm_area_struct *vma) { return true; }
-static inline void vma_lock_free(struct vm_area_struct *vma) {}
-
-#endif /* CONFIG_PER_VMA_LOCK */
-
 struct vm_area_struct *vm_area_alloc(struct mm_struct *mm)
 {
 	struct vm_area_struct *vma;
@@ -474,10 +445,6 @@ struct vm_area_struct *vm_area_alloc(struct mm_struct *mm)
 		return NULL;
 
 	vma_init(vma, mm);
-	if (!vma_lock_alloc(vma)) {
-		kmem_cache_free(vm_area_cachep, vma);
-		return NULL;
-	}
 
 	return vma;
 }
@@ -496,10 +463,7 @@ struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig)
 	 * will be reinitialized.
 	 */
 	data_race(memcpy(new, orig, sizeof(*new)));
-	if (!vma_lock_alloc(new)) {
-		kmem_cache_free(vm_area_cachep, new);
-		return NULL;
-	}
+	vma_lock_init(new);
 	INIT_LIST_HEAD(&new->anon_vma_chain);
 	vma_numab_state_init(new);
 	dup_anon_vma_name(orig, new);
@@ -511,7 +475,6 @@ void __vm_area_free(struct vm_area_struct *vma)
 {
 	vma_numab_state_free(vma);
 	free_anon_vma_name(vma);
-	vma_lock_free(vma);
 	kmem_cache_free(vm_area_cachep, vma);
 }
 
@@ -522,7 +485,7 @@ static void vm_area_free_rcu_cb(struct rcu_head *head)
 						  vm_rcu);
 
 	/* The vma should not be locked while being destroyed. */
-	VM_BUG_ON_VMA(rwsem_is_locked(&vma->vm_lock->lock), vma);
+	VM_BUG_ON_VMA(rwsem_is_locked(&vma->vm_lock.lock), vma);
 	__vm_area_free(vma);
 }
 #endif
@@ -3168,11 +3131,9 @@ void __init proc_caches_init(void)
 			sizeof(struct fs_struct), 0,
 			SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_ACCOUNT,
 			NULL);
-
-	vm_area_cachep = KMEM_CACHE(vm_area_struct, SLAB_PANIC|SLAB_ACCOUNT);
-#ifdef CONFIG_PER_VMA_LOCK
-	vma_lock_cachep = KMEM_CACHE(vma_lock, SLAB_PANIC|SLAB_ACCOUNT);
-#endif
+	vm_area_cachep = KMEM_CACHE(vm_area_struct,
+			SLAB_HWCACHE_ALIGN|SLAB_NO_MERGE|SLAB_PANIC|
+			SLAB_ACCOUNT);
 	mmap_init();
 	nsproxy_cache_init();
 }
diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
index 1d9fc97b8e80..11c2c38ca4e8 100644
--- a/tools/testing/vma/vma_internal.h
+++ b/tools/testing/vma/vma_internal.h
@@ -230,10 +230,10 @@ struct vm_area_struct {
 	/*
 	 * Can only be written (using WRITE_ONCE()) while holding both:
 	 *  - mmap_lock (in write mode)
-	 *  - vm_lock->lock (in write mode)
+	 *  - vm_lock.lock (in write mode)
 	 * Can be read reliably while holding one of:
 	 *  - mmap_lock (in read or write mode)
-	 *  - vm_lock->lock (in read or write mode)
+	 *  - vm_lock.lock (in read or write mode)
 	 * Can be read unreliably (using READ_ONCE()) for pessimistic bailout
 	 * while holding nothing (except RCU to keep the VMA struct allocated).
 	 *
@@ -242,7 +242,7 @@ struct vm_area_struct {
 	 * slowpath.
 	 */
 	unsigned int vm_lock_seq;
-	struct vma_lock *vm_lock;
+	struct vma_lock vm_lock;
 #endif
 
 	/*
@@ -408,17 +408,10 @@ static inline struct vm_area_struct *vma_next(struct vma_iterator *vmi)
 	return mas_find(&vmi->mas, ULONG_MAX);
 }
 
-static inline bool vma_lock_alloc(struct vm_area_struct *vma)
+static inline void vma_lock_init(struct vm_area_struct *vma)
 {
-	vma->vm_lock = calloc(1, sizeof(struct vma_lock));
-
-	if (!vma->vm_lock)
-		return false;
-
-	init_rwsem(&vma->vm_lock->lock);
+	init_rwsem(&vma->vm_lock.lock);
 	vma->vm_lock_seq = UINT_MAX;
-
-	return true;
 }
 
 static inline void vma_assert_write_locked(struct vm_area_struct *);
@@ -439,6 +432,7 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
 	vma->vm_ops = &vma_dummy_vm_ops;
 	INIT_LIST_HEAD(&vma->anon_vma_chain);
 	vma_mark_detached(vma, false);
+	vma_lock_init(vma);
 }
 
 static inline struct vm_area_struct *vm_area_alloc(struct mm_struct *mm)
@@ -449,10 +443,6 @@ static inline struct vm_area_struct *vm_area_alloc(struct mm_struct *mm)
 		return NULL;
 
 	vma_init(vma, mm);
-	if (!vma_lock_alloc(vma)) {
-		free(vma);
-		return NULL;
-	}
 
 	return vma;
 }
@@ -465,10 +455,7 @@ static inline struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig)
 		return NULL;
 
 	memcpy(new, orig, sizeof(*new));
-	if (!vma_lock_alloc(new)) {
-		free(new);
-		return NULL;
-	}
+	vma_lock_init(new);
 	INIT_LIST_HEAD(&new->anon_vma_chain);
 
 	return new;
@@ -638,14 +625,8 @@ static inline void mpol_put(struct mempolicy *)
 {
 }
 
-static inline void vma_lock_free(struct vm_area_struct *vma)
-{
-	free(vma->vm_lock);
-}
-
 static inline void __vm_area_free(struct vm_area_struct *vma)
 {
-	vma_lock_free(vma);
 	free(vma);
 }
 
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v3 2/5] mm: move per-vma lock into vm_area_struct

[Lorenzo Stoakes]

On Sun, Nov 17, 2024 at 12:09:28AM -0800, Suren Baghdasaryan wrote:
> Back when per-vma locks were introduces, vm_lock was moved out of
> vm_area_struct in [1] because of the performance regression caused by
> false cacheline sharing. Recent investigation [2] revealed that the
> regressions is limited to a rather old Broadwell microarchitecture and
> even there it can be mitigated by disabling adjacent cacheline
> prefetching, see [3].
> Splitting single logical structure into multiple ones leads to more
> complicated management, extra pointer dereferences and overall less
> maintainable code. When that split-away part is a lock, it complicates
> things even further. With no performance benefits, there are no reasons
> for this split. Merging the vm_lock back into vm_area_struct also allows
> vm_area_struct to use SLAB_TYPESAFE_BY_RCU later in this patchset.
> Move vm_lock back into vm_area_struct, aligning it at the cacheline
> boundary and changing the cache to be cacheline-aligned as well.

Thanks!

> With kernel compiled using defconfig, this causes VMA memory consumption
> to grow from 160 (vm_area_struct) + 40 (vm_lock) bytes to 256 bytes:
>
>     slabinfo before:
>      <name>           ... <objsize> <objperslab> <pagesperslab> : ...
>      vma_lock         ...     40  102    1 : ...
>      vm_area_struct   ...    160   51    2 : ...
>
>     slabinfo after moving vm_lock:
>      <name>           ... <objsize> <objperslab> <pagesperslab> : ...
>      vm_area_struct   ...    256   32    2 : ...
>
> Aggregate VMA memory consumption per 1000 VMAs grows from 50 to 64 pages,
> which is 5.5MB per 100000 VMAs. Note that the size of this structure is
> dependent on the kernel configuration and typically the original size is
> higher than 160 bytes. Therefore these calculations are close to the
> worst case scenario. A more realistic vm_area_struct usage before this
> change is:
>
>      <name>           ... <objsize> <objperslab> <pagesperslab> : ...
>      vma_lock         ...     40  102    1 : ...
>      vm_area_struct   ...    176   46    2 : ...
>
> Aggregate VMA memory consumption per 1000 VMAs grows from 54 to 64 pages,
> which is 3.9MB per 100000 VMAs.
> This memory consumption growth can be addressed later by optimizing the
> vm_lock.
>
> [1] https://lore.kernel.org/all/20230227173632.3292573-34-surenb@google.com/
> [2] https://lore.kernel.org/all/ZsQyI%2F087V34JoIt@xsang-OptiPlex-9020/
> [3] https://lore.kernel.org/all/CAJuCfpEisU8Lfe96AYJDZ+OM4NoPmnw9bP53cT_kbfP_pR+-2g@mail.gmail.com/
>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>

LGTM, and briefly tested in VM and looking good so:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

> ---
>  include/linux/mm.h               | 28 ++++++++++--------
>  include/linux/mm_types.h         |  6 ++--
>  kernel/fork.c                    | 49 ++++----------------------------
>  tools/testing/vma/vma_internal.h | 33 +++++----------------
>  4 files changed, 32 insertions(+), 84 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 1ba2e480ae63..737c003b0a1e 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -684,6 +684,12 @@ static inline void vma_numab_state_free(struct vm_area_struct *vma) {}
>  #endif /* CONFIG_NUMA_BALANCING */
>
>  #ifdef CONFIG_PER_VMA_LOCK
> +static inline void vma_lock_init(struct vm_area_struct *vma)
> +{
> +	init_rwsem(&vma->vm_lock.lock);
> +	vma->vm_lock_seq = UINT_MAX;
> +}
> +
>  /*
>   * Try to read-lock a vma. The function is allowed to occasionally yield false
>   * locked result to avoid performance overhead, in which case we fall back to
> @@ -701,7 +707,7 @@ static inline bool vma_start_read(struct vm_area_struct *vma)
>  	if (READ_ONCE(vma->vm_lock_seq) == READ_ONCE(vma->vm_mm->mm_lock_seq.sequence))
>  		return false;
>
> -	if (unlikely(down_read_trylock(&vma->vm_lock->lock) == 0))
> +	if (unlikely(down_read_trylock(&vma->vm_lock.lock) == 0))
>  		return false;
>
>  	/*
> @@ -716,7 +722,7 @@ static inline bool vma_start_read(struct vm_area_struct *vma)
>  	 * This pairs with RELEASE semantics in vma_end_write_all().
>  	 */
>  	if (unlikely(vma->vm_lock_seq == raw_read_seqcount(&vma->vm_mm->mm_lock_seq))) {
> -		up_read(&vma->vm_lock->lock);
> +		up_read(&vma->vm_lock.lock);
>  		return false;
>  	}
>  	return true;
> @@ -731,7 +737,7 @@ static inline bool vma_start_read(struct vm_area_struct *vma)
>  static inline void vma_start_read_locked_nested(struct vm_area_struct *vma, int subclass)
>  {
>  	mmap_assert_locked(vma->vm_mm);
> -	down_read_nested(&vma->vm_lock->lock, subclass);
> +	down_read_nested(&vma->vm_lock.lock, subclass);
>  }
>
>  /*
> @@ -743,13 +749,13 @@ static inline void vma_start_read_locked_nested(struct vm_area_struct *vma, int
>  static inline void vma_start_read_locked(struct vm_area_struct *vma)
>  {
>  	mmap_assert_locked(vma->vm_mm);
> -	down_read(&vma->vm_lock->lock);
> +	down_read(&vma->vm_lock.lock);
>  }
>
>  static inline void vma_end_read(struct vm_area_struct *vma)
>  {
>  	rcu_read_lock(); /* keeps vma alive till the end of up_read */
> -	up_read(&vma->vm_lock->lock);
> +	up_read(&vma->vm_lock.lock);
>  	rcu_read_unlock();
>  }
>
> @@ -778,7 +784,7 @@ static inline void vma_start_write(struct vm_area_struct *vma)
>  	if (__is_vma_write_locked(vma, &mm_lock_seq))
>  		return;
>
> -	down_write(&vma->vm_lock->lock);
> +	down_write(&vma->vm_lock.lock);
>  	/*
>  	 * We should use WRITE_ONCE() here because we can have concurrent reads
>  	 * from the early lockless pessimistic check in vma_start_read().
> @@ -786,7 +792,7 @@ static inline void vma_start_write(struct vm_area_struct *vma)
>  	 * we should use WRITE_ONCE() for cleanliness and to keep KCSAN happy.
>  	 */
>  	WRITE_ONCE(vma->vm_lock_seq, mm_lock_seq);
> -	up_write(&vma->vm_lock->lock);
> +	up_write(&vma->vm_lock.lock);
>  }
>
>  static inline void vma_assert_write_locked(struct vm_area_struct *vma)
> @@ -798,7 +804,7 @@ static inline void vma_assert_write_locked(struct vm_area_struct *vma)
>
>  static inline void vma_assert_locked(struct vm_area_struct *vma)
>  {
> -	if (!rwsem_is_locked(&vma->vm_lock->lock))
> +	if (!rwsem_is_locked(&vma->vm_lock.lock))
>  		vma_assert_write_locked(vma);
>  }
>
> @@ -831,6 +837,7 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
>
>  #else /* CONFIG_PER_VMA_LOCK */
>
> +static inline void vma_lock_init(struct vm_area_struct *vma) {}
>  static inline bool vma_start_read(struct vm_area_struct *vma)
>  		{ return false; }
>  static inline void vma_end_read(struct vm_area_struct *vma) {}
> @@ -865,10 +872,6 @@ static inline void assert_fault_locked(struct vm_fault *vmf)
>
>  extern const struct vm_operations_struct vma_dummy_vm_ops;
>
> -/*
> - * WARNING: vma_init does not initialize vma->vm_lock.
> - * Use vm_area_alloc()/vm_area_free() if vma needs locking.
> - */
>  static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
>  {
>  	memset(vma, 0, sizeof(*vma));
> @@ -877,6 +880,7 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
>  	INIT_LIST_HEAD(&vma->anon_vma_chain);
>  	vma_mark_detached(vma, false);
>  	vma_numab_state_init(vma);
> +	vma_lock_init(vma);
>  }
>
>  /* Use when VMA is not part of the VMA tree and needs no locking */
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 80fef38d9d64..5c4bfdcfac72 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -716,8 +716,6 @@ struct vm_area_struct {
>  	 * slowpath.
>  	 */
>  	unsigned int vm_lock_seq;
> -	/* Unstable RCU readers are allowed to read this. */
> -	struct vma_lock *vm_lock;
>  #endif
>
>  	/*
> @@ -770,6 +768,10 @@ struct vm_area_struct {
>  	struct vma_numab_state *numab_state;	/* NUMA Balancing state */
>  #endif
>  	struct vm_userfaultfd_ctx vm_userfaultfd_ctx;
> +#ifdef CONFIG_PER_VMA_LOCK
> +	/* Unstable RCU readers are allowed to read this. */
> +	struct vma_lock vm_lock ____cacheline_aligned_in_smp;
> +#endif
>  } __randomize_layout;
>
>  #ifdef CONFIG_NUMA
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 0061cf2450ef..7823797e31d2 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -436,35 +436,6 @@ static struct kmem_cache *vm_area_cachep;
>  /* SLAB cache for mm_struct structures (tsk->mm) */
>  static struct kmem_cache *mm_cachep;
>
> -#ifdef CONFIG_PER_VMA_LOCK
> -
> -/* SLAB cache for vm_area_struct.lock */
> -static struct kmem_cache *vma_lock_cachep;
> -
> -static bool vma_lock_alloc(struct vm_area_struct *vma)
> -{
> -	vma->vm_lock = kmem_cache_alloc(vma_lock_cachep, GFP_KERNEL);
> -	if (!vma->vm_lock)
> -		return false;
> -
> -	init_rwsem(&vma->vm_lock->lock);
> -	vma->vm_lock_seq = UINT_MAX;
> -
> -	return true;
> -}
> -
> -static inline void vma_lock_free(struct vm_area_struct *vma)
> -{
> -	kmem_cache_free(vma_lock_cachep, vma->vm_lock);
> -}
> -
> -#else /* CONFIG_PER_VMA_LOCK */
> -
> -static inline bool vma_lock_alloc(struct vm_area_struct *vma) { return true; }
> -static inline void vma_lock_free(struct vm_area_struct *vma) {}
> -
> -#endif /* CONFIG_PER_VMA_LOCK */
> -
>  struct vm_area_struct *vm_area_alloc(struct mm_struct *mm)
>  {
>  	struct vm_area_struct *vma;
> @@ -474,10 +445,6 @@ struct vm_area_struct *vm_area_alloc(struct mm_struct *mm)
>  		return NULL;
>
>  	vma_init(vma, mm);
> -	if (!vma_lock_alloc(vma)) {
> -		kmem_cache_free(vm_area_cachep, vma);
> -		return NULL;
> -	}
>
>  	return vma;
>  }
> @@ -496,10 +463,7 @@ struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig)
>  	 * will be reinitialized.
>  	 */
>  	data_race(memcpy(new, orig, sizeof(*new)));
> -	if (!vma_lock_alloc(new)) {
> -		kmem_cache_free(vm_area_cachep, new);
> -		return NULL;
> -	}
> +	vma_lock_init(new);
>  	INIT_LIST_HEAD(&new->anon_vma_chain);
>  	vma_numab_state_init(new);
>  	dup_anon_vma_name(orig, new);
> @@ -511,7 +475,6 @@ void __vm_area_free(struct vm_area_struct *vma)
>  {
>  	vma_numab_state_free(vma);
>  	free_anon_vma_name(vma);
> -	vma_lock_free(vma);
>  	kmem_cache_free(vm_area_cachep, vma);
>  }
>
> @@ -522,7 +485,7 @@ static void vm_area_free_rcu_cb(struct rcu_head *head)
>  						  vm_rcu);
>
>  	/* The vma should not be locked while being destroyed. */
> -	VM_BUG_ON_VMA(rwsem_is_locked(&vma->vm_lock->lock), vma);
> +	VM_BUG_ON_VMA(rwsem_is_locked(&vma->vm_lock.lock), vma);
>  	__vm_area_free(vma);
>  }
>  #endif
> @@ -3168,11 +3131,9 @@ void __init proc_caches_init(void)
>  			sizeof(struct fs_struct), 0,
>  			SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_ACCOUNT,
>  			NULL);
> -
> -	vm_area_cachep = KMEM_CACHE(vm_area_struct, SLAB_PANIC|SLAB_ACCOUNT);
> -#ifdef CONFIG_PER_VMA_LOCK
> -	vma_lock_cachep = KMEM_CACHE(vma_lock, SLAB_PANIC|SLAB_ACCOUNT);
> -#endif
> +	vm_area_cachep = KMEM_CACHE(vm_area_struct,
> +			SLAB_HWCACHE_ALIGN|SLAB_NO_MERGE|SLAB_PANIC|
> +			SLAB_ACCOUNT);
>  	mmap_init();
>  	nsproxy_cache_init();
>  }
> diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
> index 1d9fc97b8e80..11c2c38ca4e8 100644
> --- a/tools/testing/vma/vma_internal.h
> +++ b/tools/testing/vma/vma_internal.h
> @@ -230,10 +230,10 @@ struct vm_area_struct {
>  	/*
>  	 * Can only be written (using WRITE_ONCE()) while holding both:
>  	 *  - mmap_lock (in write mode)
> -	 *  - vm_lock->lock (in write mode)
> +	 *  - vm_lock.lock (in write mode)
>  	 * Can be read reliably while holding one of:
>  	 *  - mmap_lock (in read or write mode)
> -	 *  - vm_lock->lock (in read or write mode)
> +	 *  - vm_lock.lock (in read or write mode)
>  	 * Can be read unreliably (using READ_ONCE()) for pessimistic bailout
>  	 * while holding nothing (except RCU to keep the VMA struct allocated).
>  	 *
> @@ -242,7 +242,7 @@ struct vm_area_struct {
>  	 * slowpath.
>  	 */
>  	unsigned int vm_lock_seq;
> -	struct vma_lock *vm_lock;
> +	struct vma_lock vm_lock;
>  #endif
>
>  	/*
> @@ -408,17 +408,10 @@ static inline struct vm_area_struct *vma_next(struct vma_iterator *vmi)
>  	return mas_find(&vmi->mas, ULONG_MAX);
>  }
>
> -static inline bool vma_lock_alloc(struct vm_area_struct *vma)
> +static inline void vma_lock_init(struct vm_area_struct *vma)
>  {
> -	vma->vm_lock = calloc(1, sizeof(struct vma_lock));
> -
> -	if (!vma->vm_lock)
> -		return false;
> -
> -	init_rwsem(&vma->vm_lock->lock);
> +	init_rwsem(&vma->vm_lock.lock);
>  	vma->vm_lock_seq = UINT_MAX;
> -
> -	return true;
>  }
>
>  static inline void vma_assert_write_locked(struct vm_area_struct *);
> @@ -439,6 +432,7 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
>  	vma->vm_ops = &vma_dummy_vm_ops;
>  	INIT_LIST_HEAD(&vma->anon_vma_chain);
>  	vma_mark_detached(vma, false);
> +	vma_lock_init(vma);
>  }
>
>  static inline struct vm_area_struct *vm_area_alloc(struct mm_struct *mm)
> @@ -449,10 +443,6 @@ static inline struct vm_area_struct *vm_area_alloc(struct mm_struct *mm)
>  		return NULL;
>
>  	vma_init(vma, mm);
> -	if (!vma_lock_alloc(vma)) {
> -		free(vma);
> -		return NULL;
> -	}
>
>  	return vma;
>  }
> @@ -465,10 +455,7 @@ static inline struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig)
>  		return NULL;
>
>  	memcpy(new, orig, sizeof(*new));
> -	if (!vma_lock_alloc(new)) {
> -		free(new);
> -		return NULL;
> -	}
> +	vma_lock_init(new);
>  	INIT_LIST_HEAD(&new->anon_vma_chain);
>
>  	return new;
> @@ -638,14 +625,8 @@ static inline void mpol_put(struct mempolicy *)
>  {
>  }
>
> -static inline void vma_lock_free(struct vm_area_struct *vma)
> -{
> -	free(vma->vm_lock);
> -}
> -
>  static inline void __vm_area_free(struct vm_area_struct *vma)
>  {
> -	vma_lock_free(vma);
>  	free(vma);
>  }
>
> --
> 2.47.0.338.g60cca15819-goog
>

[PATCH v3 3/5] mm: mark vma as detached until it's added into vma tree

[Suren Baghdasaryan]

Current implementation does not set detached flag when a VMA is first
allocated. This does not represent the real state of the VMA, which is
detached until it is added into mm's VMA tree. Fix this by marking new
VMAs as detached and resetting detached flag only after VMA is added
into a tree.
Introduce vma_mark_attached() to make the API more readable and to
simplify possible future cleanup when vma->vm_mm might be used to
indicate detached vma and vma_mark_attached() will need an additional
mm parameter.

Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
 include/linux/mm.h               | 27 ++++++++++++++++++++-------
 kernel/fork.c                    |  4 ++++
 mm/memory.c                      |  2 +-
 mm/vma.c                         |  6 +++---
 mm/vma.h                         |  2 ++
 tools/testing/vma/vma_internal.h | 17 ++++++++++++-----
 6 files changed, 42 insertions(+), 16 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 737c003b0a1e..dd1b6190df28 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -808,12 +808,21 @@ static inline void vma_assert_locked(struct vm_area_struct *vma)
 		vma_assert_write_locked(vma);
 }
 
-static inline void vma_mark_detached(struct vm_area_struct *vma, bool detached)
+static inline void vma_mark_attached(struct vm_area_struct *vma)
+{
+	vma->detached = false;
+}
+
+static inline void vma_mark_detached(struct vm_area_struct *vma)
 {
 	/* When detaching vma should be write-locked */
-	if (detached)
-		vma_assert_write_locked(vma);
-	vma->detached = detached;
+	vma_assert_write_locked(vma);
+	vma->detached = true;
+}
+
+static inline bool is_vma_detached(struct vm_area_struct *vma)
+{
+	return vma->detached;
 }
 
 static inline void release_fault_lock(struct vm_fault *vmf)
@@ -844,8 +853,8 @@ static inline void vma_end_read(struct vm_area_struct *vma) {}
 static inline void vma_start_write(struct vm_area_struct *vma) {}
 static inline void vma_assert_write_locked(struct vm_area_struct *vma)
 		{ mmap_assert_write_locked(vma->vm_mm); }
-static inline void vma_mark_detached(struct vm_area_struct *vma,
-				     bool detached) {}
+static inline void vma_mark_attached(struct vm_area_struct *vma) {}
+static inline void vma_mark_detached(struct vm_area_struct *vma) {}
 
 static inline struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
 		unsigned long address)
@@ -878,7 +887,10 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
 	vma->vm_mm = mm;
 	vma->vm_ops = &vma_dummy_vm_ops;
 	INIT_LIST_HEAD(&vma->anon_vma_chain);
-	vma_mark_detached(vma, false);
+#ifdef CONFIG_PER_VMA_LOCK
+	/* vma is not locked, can't use vma_mark_detached() */
+	vma->detached = true;
+#endif
 	vma_numab_state_init(vma);
 	vma_lock_init(vma);
 }
@@ -1073,6 +1085,7 @@ static inline int vma_iter_bulk_store(struct vma_iterator *vmi,
 	if (unlikely(mas_is_err(&vmi->mas)))
 		return -ENOMEM;
 
+	vma_mark_attached(vma);
 	return 0;
 }
 
diff --git a/kernel/fork.c b/kernel/fork.c
index 7823797e31d2..f0cec673583c 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -465,6 +465,10 @@ struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig)
 	data_race(memcpy(new, orig, sizeof(*new)));
 	vma_lock_init(new);
 	INIT_LIST_HEAD(&new->anon_vma_chain);
+#ifdef CONFIG_PER_VMA_LOCK
+	/* vma is not locked, can't use vma_mark_detached() */
+	new->detached = true;
+#endif
 	vma_numab_state_init(new);
 	dup_anon_vma_name(orig, new);
 
diff --git a/mm/memory.c b/mm/memory.c
index 209885a4134f..d0197a0c0996 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -6279,7 +6279,7 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
 		goto inval;
 
 	/* Check if the VMA got isolated after we found it */
-	if (vma->detached) {
+	if (is_vma_detached(vma)) {
 		vma_end_read(vma);
 		count_vm_vma_lock_event(VMA_LOCK_MISS);
 		/* The area was replaced with another one */
diff --git a/mm/vma.c b/mm/vma.c
index 8a454a7bbc80..73104d434567 100644
--- a/mm/vma.c
+++ b/mm/vma.c
@@ -295,7 +295,7 @@ static void vma_complete(struct vma_prepare *vp, struct vma_iterator *vmi,
 
 	if (vp->remove) {
 again:
-		vma_mark_detached(vp->remove, true);
+		vma_mark_detached(vp->remove);
 		if (vp->file) {
 			uprobe_munmap(vp->remove, vp->remove->vm_start,
 				      vp->remove->vm_end);
@@ -1220,7 +1220,7 @@ static void reattach_vmas(struct ma_state *mas_detach)
 
 	mas_set(mas_detach, 0);
 	mas_for_each(mas_detach, vma, ULONG_MAX)
-		vma_mark_detached(vma, false);
+		vma_mark_attached(vma);
 
 	__mt_destroy(mas_detach->tree);
 }
@@ -1295,7 +1295,7 @@ static int vms_gather_munmap_vmas(struct vma_munmap_struct *vms,
 		if (error)
 			goto munmap_gather_failed;
 
-		vma_mark_detached(next, true);
+		vma_mark_detached(next);
 		nrpages = vma_pages(next);
 
 		vms->nr_pages += nrpages;
diff --git a/mm/vma.h b/mm/vma.h
index 388d34748674..2e680f357ace 100644
--- a/mm/vma.h
+++ b/mm/vma.h
@@ -162,6 +162,7 @@ static inline int vma_iter_store_gfp(struct vma_iterator *vmi,
 	if (unlikely(mas_is_err(&vmi->mas)))
 		return -ENOMEM;
 
+	vma_mark_attached(vma);
 	return 0;
 }
 
@@ -385,6 +386,7 @@ static inline void vma_iter_store(struct vma_iterator *vmi,
 
 	__mas_set_range(&vmi->mas, vma->vm_start, vma->vm_end - 1);
 	mas_store_prealloc(&vmi->mas, vma);
+	vma_mark_attached(vma);
 }
 
 static inline unsigned long vma_iter_addr(struct vma_iterator *vmi)
diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
index 11c2c38ca4e8..2fed366d20ef 100644
--- a/tools/testing/vma/vma_internal.h
+++ b/tools/testing/vma/vma_internal.h
@@ -414,13 +414,17 @@ static inline void vma_lock_init(struct vm_area_struct *vma)
 	vma->vm_lock_seq = UINT_MAX;
 }
 
+static inline void vma_mark_attached(struct vm_area_struct *vma)
+{
+	vma->detached = false;
+}
+
 static inline void vma_assert_write_locked(struct vm_area_struct *);
-static inline void vma_mark_detached(struct vm_area_struct *vma, bool detached)
+static inline void vma_mark_detached(struct vm_area_struct *vma)
 {
 	/* When detaching vma should be write-locked */
-	if (detached)
-		vma_assert_write_locked(vma);
-	vma->detached = detached;
+	vma_assert_write_locked(vma);
+	vma->detached = true;
 }
 
 extern const struct vm_operations_struct vma_dummy_vm_ops;
@@ -431,7 +435,8 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
 	vma->vm_mm = mm;
 	vma->vm_ops = &vma_dummy_vm_ops;
 	INIT_LIST_HEAD(&vma->anon_vma_chain);
-	vma_mark_detached(vma, false);
+	/* vma is not locked, can't use vma_mark_detached() */
+	vma->detached = true;
 	vma_lock_init(vma);
 }
 
@@ -457,6 +462,8 @@ static inline struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig)
 	memcpy(new, orig, sizeof(*new));
 	vma_lock_init(new);
 	INIT_LIST_HEAD(&new->anon_vma_chain);
+	/* vma is not locked, can't use vma_mark_detached() */
+	new->detached = true;
 
 	return new;
 }
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v3 3/5] mm: mark vma as detached until it's added into vma tree

[Lorenzo Stoakes]

So, this causes VMAs which are already attached to be marked attached
again, and when the check added in "mm: make vma cache
SLAB_TYPESAFE_BY_RCU", which is:

static inline void vma_mark_attached(struct vm_area_struct *vma)
{
	/* vma shoudn't be already attached */
	VM_BUG_ON_VMA(!vma->detached, vma); <-------- here

	...
}

Is executed, it fails and asserts on boot, as per the below (I ran
addr2line and identified this as the cause).

[    0.615256] vma ffff88810086e000 start 00007ffedf98e000 end 00007ffffffff000 mm ffff888101bf0000
[    0.615256] prot 8000000000000025 anon_vma ffff88810026c000 vm_ops 0000000000000000
[    0.615256] pgoff 7fffffffe file 0000000000000000 private_data 0000000000000000
[    0.615256] flags: 0x118173(read|write|mayread|maywrite|mayexec|growsdown|seqread|randread|account)
[    0.616232] Oops: invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
[    0.616416] CPU: 3 UID: 0 PID: 1 Comm: init Not tainted 6.12.0-rc6+ #9
[    0.616618] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.3-1-1 04/01/2014
[    0.616914] RIP: 0010:commit_merge+0x361/0x390
[    0.617059] Code: 28 e9 58 fd ff ff 49 39 44 24 10 72 c7 e9 81 fe ff ff 48 39 57 10 0f 82 1d ff ff ff e9 1c ff ff ff 48 89 c7 e8 70 0
[    0.617609] RSP: 0018:ffffc90000013a48 EFLAGS: 00010292
[    0.617778] RAX: 0000000000000138 RBX: ffffc90000013b68 RCX: 0000000000000000
[    0.617995] RDX: 0000000000000003 RSI: ffffc900000138d0 RDI: 0000000000000001
[    0.618209] RBP: 0000000000000000 R08: 00000000ffffdfff R09: ffffffff82b089a8
[    0.618429] R10: 0000000000000003 R11: 30203a7367616c66 R12: 0000000000000000
[    0.618653] R13: 0000000000000001 R14: 0000000000000000 R15: ffffc90000013a58
[    0.618846] FS:  0000000000000000(0000) GS:ffff888263d80000(0000) knlGS:0000000000000000
[    0.619041] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    0.619186] CR2: 0000000000000000 CR3: 0000000101c74000 CR4: 0000000000750ef0
[    0.619357] PKRU: 55555554
[    0.619425] Call Trace:
[    0.619491]  <TASK>
[    0.619546]  ? __die_body.cold+0x19/0x2a
[    0.619644]  ? die+0x29/0x50
[    0.619719]  ? do_trap+0xc5/0x110
[    0.619808]  ? do_error_trap+0x60/0x80
[    0.619901]  ? commit_merge+0x361/0x390
[    0.619995]  ? exc_invalid_op+0x51/0x70
[    0.620092]  ? commit_merge+0x361/0x390
[    0.620185]  ? asm_exc_invalid_op+0x1a/0x20
[    0.620288]  ? commit_merge+0x361/0x390
[    0.620383]  ? commit_merge+0x360/0x390
[    0.620478]  vma_expand+0xd0/0x1a0
[    0.620563]  relocate_vma_down+0xe8/0x1e0
[    0.620664]  setup_arg_pages+0x1f6/0x360
[    0.620783]  load_elf_binary+0x37b/0x1720
[    0.620912]  ? __kernel_read+0x187/0x2e0
[    0.621038]  ? load_misc_binary+0x225/0x2f0
[    0.621173]  bprm_execve+0x22e/0x5b0
[    0.621288]  kernel_execve+0x10b/0x140
[    0.621406]  try_to_run_init_process+0xa/0x2e
[    0.621545]  ? __pfx_kernel_init+0x10/0x10
[    0.621675]  kernel_init+0xde/0x130
[    0.621796]  ret_from_fork+0x2c/0x50
[    0.621914]  ? __pfx_kernel_init+0x10/0x10
[    0.622046]  ret_from_fork_asm+0x1a/0x30
[    0.622174]  </TASK>
[    0.622248] Modules linked in:
[    0.622356] ---[ end trace 0000000000000000 ]---
[    0.622502] RIP: 0010:commit_merge+0x361/0x390
[    0.622643] Code: 28 e9 58 fd ff ff 49 39 44 24 10 72 c7 e9 81 fe ff ff 48 39 57 10 0f 82 1d ff ff ff e9 1c ff ff ff 48 89 c7 e8 70 0
[    0.623213] RSP: 0018:ffffc90000013a48 EFLAGS: 00010292
[    0.623381] RAX: 0000000000000138 RBX: ffffc90000013b68 RCX: 0000000000000000
[    0.623596] RDX: 0000000000000003 RSI: ffffc900000138d0 RDI: 0000000000000001
[    0.623825] RBP: 0000000000000000 R08: 00000000ffffdfff R09: ffffffff82b089a8
[    0.624045] R10: 0000000000000003 R11: 30203a7367616c66 R12: 0000000000000000
[    0.624268] R13: 0000000000000001 R14: 0000000000000000 R15: ffffc90000013a58
[    0.624484] FS:  0000000000000000(0000) GS:ffff888263d80000(0000) knlGS:0000000000000000
[    0.624746] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    0.624926] CR2: 0000000000000000 CR3: 0000000101c74000 CR4: 0000000000750ef0
[    0.625149] PKRU: 55555554
[    0.625244] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
[    0.625545] Kernel Offset: disabled
[    0.625658] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---

If I add code to detach first in relocate_vma_down(), then
expand_downwards() has the same issue. It seems this code doesn't account
for such cases.

If I add code to fix _this_ then a VMA merge triggers it and... I think
this is just fundamentally broken...

There are cases where we change the size of an existing VMA and overwrite
stuff in the maple tree, this is normal, and we do it to an attached VMA.

So actually perhaps... we should just drop this check altogether?

My workarounds are essentially to mark detached immediately prior to
vma_iter_store() which seems to defeat the purpose :P

On Sun, Nov 17, 2024 at 12:09:29AM -0800, Suren Baghdasaryan wrote:
> Current implementation does not set detached flag when a VMA is first
> allocated. This does not represent the real state of the VMA, which is
> detached until it is added into mm's VMA tree. Fix this by marking new
> VMAs as detached and resetting detached flag only after VMA is added
> into a tree.
> Introduce vma_mark_attached() to make the API more readable and to
> simplify possible future cleanup when vma->vm_mm might be used to
> indicate detached vma and vma_mark_attached() will need an additional
> mm parameter.
>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> ---
>  include/linux/mm.h               | 27 ++++++++++++++++++++-------
>  kernel/fork.c                    |  4 ++++
>  mm/memory.c                      |  2 +-
>  mm/vma.c                         |  6 +++---
>  mm/vma.h                         |  2 ++
>  tools/testing/vma/vma_internal.h | 17 ++++++++++++-----
>  6 files changed, 42 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 737c003b0a1e..dd1b6190df28 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -808,12 +808,21 @@ static inline void vma_assert_locked(struct vm_area_struct *vma)
>  		vma_assert_write_locked(vma);
>  }
>
> -static inline void vma_mark_detached(struct vm_area_struct *vma, bool detached)
> +static inline void vma_mark_attached(struct vm_area_struct *vma)
> +{
> +	vma->detached = false;
> +}

We should definitely add the

	VM_BUG_ON_VMA(!vma->detached, vma);

Check that is added in "mm: make vma cache SLAB_TYPESAFE_BY_RCU" here
instead, if we want it.

But as per above I'm not sure we do...

> +
> +static inline void vma_mark_detached(struct vm_area_struct *vma)
>  {
>  	/* When detaching vma should be write-locked */
> -	if (detached)
> -		vma_assert_write_locked(vma);
> -	vma->detached = detached;
> +	vma_assert_write_locked(vma);
> +	vma->detached = true;
> +}

Do we want to assert it was attached before? Then again given the attached
assert probably not :>)

> +
> +static inline bool is_vma_detached(struct vm_area_struct *vma)
> +{
> +	return vma->detached;
>  }
>
>  static inline void release_fault_lock(struct vm_fault *vmf)
> @@ -844,8 +853,8 @@ static inline void vma_end_read(struct vm_area_struct *vma) {}
>  static inline void vma_start_write(struct vm_area_struct *vma) {}
>  static inline void vma_assert_write_locked(struct vm_area_struct *vma)
>  		{ mmap_assert_write_locked(vma->vm_mm); }
> -static inline void vma_mark_detached(struct vm_area_struct *vma,
> -				     bool detached) {}
> +static inline void vma_mark_attached(struct vm_area_struct *vma) {}
> +static inline void vma_mark_detached(struct vm_area_struct *vma) {}
>
>  static inline struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
>  		unsigned long address)
> @@ -878,7 +887,10 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
>  	vma->vm_mm = mm;
>  	vma->vm_ops = &vma_dummy_vm_ops;
>  	INIT_LIST_HEAD(&vma->anon_vma_chain);
> -	vma_mark_detached(vma, false);
> +#ifdef CONFIG_PER_VMA_LOCK
> +	/* vma is not locked, can't use vma_mark_detached() */
> +	vma->detached = true;
> +#endif
>  	vma_numab_state_init(vma);
>  	vma_lock_init(vma);
>  }
> @@ -1073,6 +1085,7 @@ static inline int vma_iter_bulk_store(struct vma_iterator *vmi,
>  	if (unlikely(mas_is_err(&vmi->mas)))
>  		return -ENOMEM;
>
> +	vma_mark_attached(vma);
>  	return 0;
>  }
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 7823797e31d2..f0cec673583c 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -465,6 +465,10 @@ struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig)
>  	data_race(memcpy(new, orig, sizeof(*new)));
>  	vma_lock_init(new);
>  	INIT_LIST_HEAD(&new->anon_vma_chain);
> +#ifdef CONFIG_PER_VMA_LOCK
> +	/* vma is not locked, can't use vma_mark_detached() */
> +	new->detached = true;
> +#endif
>  	vma_numab_state_init(new);
>  	dup_anon_vma_name(orig, new);
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 209885a4134f..d0197a0c0996 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -6279,7 +6279,7 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
>  		goto inval;
>
>  	/* Check if the VMA got isolated after we found it */
> -	if (vma->detached) {
> +	if (is_vma_detached(vma)) {
>  		vma_end_read(vma);
>  		count_vm_vma_lock_event(VMA_LOCK_MISS);
>  		/* The area was replaced with another one */
> diff --git a/mm/vma.c b/mm/vma.c
> index 8a454a7bbc80..73104d434567 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -295,7 +295,7 @@ static void vma_complete(struct vma_prepare *vp, struct vma_iterator *vmi,
>
>  	if (vp->remove) {
>  again:
> -		vma_mark_detached(vp->remove, true);
> +		vma_mark_detached(vp->remove);
>  		if (vp->file) {
>  			uprobe_munmap(vp->remove, vp->remove->vm_start,
>  				      vp->remove->vm_end);
> @@ -1220,7 +1220,7 @@ static void reattach_vmas(struct ma_state *mas_detach)
>
>  	mas_set(mas_detach, 0);
>  	mas_for_each(mas_detach, vma, ULONG_MAX)
> -		vma_mark_detached(vma, false);
> +		vma_mark_attached(vma);
>
>  	__mt_destroy(mas_detach->tree);
>  }

This is on a subtle error handling code path, we should definitely do some
careful checking of this (it might be nice to add some to the vma.c
userland tests...)

> @@ -1295,7 +1295,7 @@ static int vms_gather_munmap_vmas(struct vma_munmap_struct *vms,
>  		if (error)
>  			goto munmap_gather_failed;
>
> -		vma_mark_detached(next, true);
> +		vma_mark_detached(next);
>  		nrpages = vma_pages(next);
>
>  		vms->nr_pages += nrpages;
> diff --git a/mm/vma.h b/mm/vma.h
> index 388d34748674..2e680f357ace 100644
> --- a/mm/vma.h
> +++ b/mm/vma.h
> @@ -162,6 +162,7 @@ static inline int vma_iter_store_gfp(struct vma_iterator *vmi,
>  	if (unlikely(mas_is_err(&vmi->mas)))
>  		return -ENOMEM;
>
> +	vma_mark_attached(vma);
>  	return 0;
>  }
>
> @@ -385,6 +386,7 @@ static inline void vma_iter_store(struct vma_iterator *vmi,
>
>  	__mas_set_range(&vmi->mas, vma->vm_start, vma->vm_end - 1);
>  	mas_store_prealloc(&vmi->mas, vma);
> +	vma_mark_attached(vma);
>  }
>
>  static inline unsigned long vma_iter_addr(struct vma_iterator *vmi)
> diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
> index 11c2c38ca4e8..2fed366d20ef 100644
> --- a/tools/testing/vma/vma_internal.h
> +++ b/tools/testing/vma/vma_internal.h
> @@ -414,13 +414,17 @@ static inline void vma_lock_init(struct vm_area_struct *vma)
>  	vma->vm_lock_seq = UINT_MAX;
>  }
>
> +static inline void vma_mark_attached(struct vm_area_struct *vma)
> +{
> +	vma->detached = false;
> +}
> +
>  static inline void vma_assert_write_locked(struct vm_area_struct *);
> -static inline void vma_mark_detached(struct vm_area_struct *vma, bool detached)
> +static inline void vma_mark_detached(struct vm_area_struct *vma)
>  {
>  	/* When detaching vma should be write-locked */
> -	if (detached)
> -		vma_assert_write_locked(vma);
> -	vma->detached = detached;
> +	vma_assert_write_locked(vma);
> +	vma->detached = true;
>  }
>
>  extern const struct vm_operations_struct vma_dummy_vm_ops;
> @@ -431,7 +435,8 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
>  	vma->vm_mm = mm;
>  	vma->vm_ops = &vma_dummy_vm_ops;
>  	INIT_LIST_HEAD(&vma->anon_vma_chain);
> -	vma_mark_detached(vma, false);
> +	/* vma is not locked, can't use vma_mark_detached() */
> +	vma->detached = true;
>  	vma_lock_init(vma);
>  }
>
> @@ -457,6 +462,8 @@ static inline struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig)
>  	memcpy(new, orig, sizeof(*new));
>  	vma_lock_init(new);
>  	INIT_LIST_HEAD(&new->anon_vma_chain);
> +	/* vma is not locked, can't use vma_mark_detached() */
> +	new->detached = true;
>
>  	return new;
>  }
> --
> 2.47.0.338.g60cca15819-goog
>

Re: [PATCH v3 3/5] mm: mark vma as detached until it's added into vma tree

[Suren Baghdasaryan]

On Mon, Nov 18, 2024 at 6:10 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> So, this causes VMAs which are already attached to be marked attached
> again, and when the check added in "mm: make vma cache
> SLAB_TYPESAFE_BY_RCU", which is:
>
> static inline void vma_mark_attached(struct vm_area_struct *vma)
> {
>         /* vma shoudn't be already attached */
>         VM_BUG_ON_VMA(!vma->detached, vma); <-------- here
>
>         ...
> }
>
> Is executed, it fails and asserts on boot, as per the below (I ran
> addr2line and identified this as the cause).
>
> [    0.615256] vma ffff88810086e000 start 00007ffedf98e000 end 00007ffffffff000 mm ffff888101bf0000
> [    0.615256] prot 8000000000000025 anon_vma ffff88810026c000 vm_ops 0000000000000000
> [    0.615256] pgoff 7fffffffe file 0000000000000000 private_data 0000000000000000
> [    0.615256] flags: 0x118173(read|write|mayread|maywrite|mayexec|growsdown|seqread|randread|account)
> [    0.616232] Oops: invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
> [    0.616416] CPU: 3 UID: 0 PID: 1 Comm: init Not tainted 6.12.0-rc6+ #9
> [    0.616618] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.3-1-1 04/01/2014
> [    0.616914] RIP: 0010:commit_merge+0x361/0x390
> [    0.617059] Code: 28 e9 58 fd ff ff 49 39 44 24 10 72 c7 e9 81 fe ff ff 48 39 57 10 0f 82 1d ff ff ff e9 1c ff ff ff 48 89 c7 e8 70 0
> [    0.617609] RSP: 0018:ffffc90000013a48 EFLAGS: 00010292
> [    0.617778] RAX: 0000000000000138 RBX: ffffc90000013b68 RCX: 0000000000000000
> [    0.617995] RDX: 0000000000000003 RSI: ffffc900000138d0 RDI: 0000000000000001
> [    0.618209] RBP: 0000000000000000 R08: 00000000ffffdfff R09: ffffffff82b089a8
> [    0.618429] R10: 0000000000000003 R11: 30203a7367616c66 R12: 0000000000000000
> [    0.618653] R13: 0000000000000001 R14: 0000000000000000 R15: ffffc90000013a58
> [    0.618846] FS:  0000000000000000(0000) GS:ffff888263d80000(0000) knlGS:0000000000000000
> [    0.619041] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    0.619186] CR2: 0000000000000000 CR3: 0000000101c74000 CR4: 0000000000750ef0
> [    0.619357] PKRU: 55555554
> [    0.619425] Call Trace:
> [    0.619491]  <TASK>
> [    0.619546]  ? __die_body.cold+0x19/0x2a
> [    0.619644]  ? die+0x29/0x50
> [    0.619719]  ? do_trap+0xc5/0x110
> [    0.619808]  ? do_error_trap+0x60/0x80
> [    0.619901]  ? commit_merge+0x361/0x390
> [    0.619995]  ? exc_invalid_op+0x51/0x70
> [    0.620092]  ? commit_merge+0x361/0x390
> [    0.620185]  ? asm_exc_invalid_op+0x1a/0x20
> [    0.620288]  ? commit_merge+0x361/0x390
> [    0.620383]  ? commit_merge+0x360/0x390
> [    0.620478]  vma_expand+0xd0/0x1a0
> [    0.620563]  relocate_vma_down+0xe8/0x1e0
> [    0.620664]  setup_arg_pages+0x1f6/0x360
> [    0.620783]  load_elf_binary+0x37b/0x1720
> [    0.620912]  ? __kernel_read+0x187/0x2e0
> [    0.621038]  ? load_misc_binary+0x225/0x2f0
> [    0.621173]  bprm_execve+0x22e/0x5b0
> [    0.621288]  kernel_execve+0x10b/0x140
> [    0.621406]  try_to_run_init_process+0xa/0x2e
> [    0.621545]  ? __pfx_kernel_init+0x10/0x10
> [    0.621675]  kernel_init+0xde/0x130
> [    0.621796]  ret_from_fork+0x2c/0x50
> [    0.621914]  ? __pfx_kernel_init+0x10/0x10
> [    0.622046]  ret_from_fork_asm+0x1a/0x30
> [    0.622174]  </TASK>
> [    0.622248] Modules linked in:
> [    0.622356] ---[ end trace 0000000000000000 ]---
> [    0.622502] RIP: 0010:commit_merge+0x361/0x390
> [    0.622643] Code: 28 e9 58 fd ff ff 49 39 44 24 10 72 c7 e9 81 fe ff ff 48 39 57 10 0f 82 1d ff ff ff e9 1c ff ff ff 48 89 c7 e8 70 0
> [    0.623213] RSP: 0018:ffffc90000013a48 EFLAGS: 00010292
> [    0.623381] RAX: 0000000000000138 RBX: ffffc90000013b68 RCX: 0000000000000000
> [    0.623596] RDX: 0000000000000003 RSI: ffffc900000138d0 RDI: 0000000000000001
> [    0.623825] RBP: 0000000000000000 R08: 00000000ffffdfff R09: ffffffff82b089a8
> [    0.624045] R10: 0000000000000003 R11: 30203a7367616c66 R12: 0000000000000000
> [    0.624268] R13: 0000000000000001 R14: 0000000000000000 R15: ffffc90000013a58
> [    0.624484] FS:  0000000000000000(0000) GS:ffff888263d80000(0000) knlGS:0000000000000000
> [    0.624746] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    0.624926] CR2: 0000000000000000 CR3: 0000000101c74000 CR4: 0000000000750ef0
> [    0.625149] PKRU: 55555554
> [    0.625244] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> [    0.625545] Kernel Offset: disabled
> [    0.625658] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---
>
> If I add code to detach first in relocate_vma_down(), then
> expand_downwards() has the same issue. It seems this code doesn't account
> for such cases.
>
> If I add code to fix _this_ then a VMA merge triggers it and... I think
> this is just fundamentally broken...
>
> There are cases where we change the size of an existing VMA and overwrite
> stuff in the maple tree, this is normal, and we do it to an attached VMA.
>
> So actually perhaps... we should just drop this check altogether?
>
> My workarounds are essentially to mark detached immediately prior to
> vma_iter_store() which seems to defeat the purpose :P

I realized that this assertion was added at a later stage of the patch
and I tested it using the same config that I use for performance
testing, which did not have CONFIG_DEBUG_VM enabled. Sorry about that.
I didn't realize we are modifying and reinserting the vma without
marking it detached, however these cases are not an issue for vma
reuse because we do not free the vma in the process. I think the
following should work fine:

static inline void vma_mark_attached(struct vm_area_struct *vma)
{
        /* If vma is write-locked then it's already attached */
        if (down_write_trylock(&vma->vm_lock.lock)) {
                vma->detached = false;
                up_write(&vma->vm_lock.lock);
        }
}

I'll think some more about edge cases and will post the new patchset
with the fix.
Thanks for reviewing and testing, Lorenzo!
Suren.

>
> On Sun, Nov 17, 2024 at 12:09:29AM -0800, Suren Baghdasaryan wrote:
> > Current implementation does not set detached flag when a VMA is first
> > allocated. This does not represent the real state of the VMA, which is
> > detached until it is added into mm's VMA tree. Fix this by marking new
> > VMAs as detached and resetting detached flag only after VMA is added
> > into a tree.
> > Introduce vma_mark_attached() to make the API more readable and to
> > simplify possible future cleanup when vma->vm_mm might be used to
> > indicate detached vma and vma_mark_attached() will need an additional
> > mm parameter.
> >
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > ---
> >  include/linux/mm.h               | 27 ++++++++++++++++++++-------
> >  kernel/fork.c                    |  4 ++++
> >  mm/memory.c                      |  2 +-
> >  mm/vma.c                         |  6 +++---
> >  mm/vma.h                         |  2 ++
> >  tools/testing/vma/vma_internal.h | 17 ++++++++++++-----
> >  6 files changed, 42 insertions(+), 16 deletions(-)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 737c003b0a1e..dd1b6190df28 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -808,12 +808,21 @@ static inline void vma_assert_locked(struct vm_area_struct *vma)
> >               vma_assert_write_locked(vma);
> >  }
> >
> > -static inline void vma_mark_detached(struct vm_area_struct *vma, bool detached)
> > +static inline void vma_mark_attached(struct vm_area_struct *vma)
> > +{
> > +     vma->detached = false;
> > +}
>
> We should definitely add the
>
>         VM_BUG_ON_VMA(!vma->detached, vma);
>
> Check that is added in "mm: make vma cache SLAB_TYPESAFE_BY_RCU" here
> instead, if we want it.
>
> But as per above I'm not sure we do...
>
> > +
> > +static inline void vma_mark_detached(struct vm_area_struct *vma)
> >  {
> >       /* When detaching vma should be write-locked */
> > -     if (detached)
> > -             vma_assert_write_locked(vma);
> > -     vma->detached = detached;
> > +     vma_assert_write_locked(vma);
> > +     vma->detached = true;
> > +}
>
> Do we want to assert it was attached before? Then again given the attached
> assert probably not :>)
>
> > +
> > +static inline bool is_vma_detached(struct vm_area_struct *vma)
> > +{
> > +     return vma->detached;
> >  }
> >
> >  static inline void release_fault_lock(struct vm_fault *vmf)
> > @@ -844,8 +853,8 @@ static inline void vma_end_read(struct vm_area_struct *vma) {}
> >  static inline void vma_start_write(struct vm_area_struct *vma) {}
> >  static inline void vma_assert_write_locked(struct vm_area_struct *vma)
> >               { mmap_assert_write_locked(vma->vm_mm); }
> > -static inline void vma_mark_detached(struct vm_area_struct *vma,
> > -                                  bool detached) {}
> > +static inline void vma_mark_attached(struct vm_area_struct *vma) {}
> > +static inline void vma_mark_detached(struct vm_area_struct *vma) {}
> >
> >  static inline struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
> >               unsigned long address)
> > @@ -878,7 +887,10 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
> >       vma->vm_mm = mm;
> >       vma->vm_ops = &vma_dummy_vm_ops;
> >       INIT_LIST_HEAD(&vma->anon_vma_chain);
> > -     vma_mark_detached(vma, false);
> > +#ifdef CONFIG_PER_VMA_LOCK
> > +     /* vma is not locked, can't use vma_mark_detached() */
> > +     vma->detached = true;
> > +#endif
> >       vma_numab_state_init(vma);
> >       vma_lock_init(vma);
> >  }
> > @@ -1073,6 +1085,7 @@ static inline int vma_iter_bulk_store(struct vma_iterator *vmi,
> >       if (unlikely(mas_is_err(&vmi->mas)))
> >               return -ENOMEM;
> >
> > +     vma_mark_attached(vma);
> >       return 0;
> >  }
> >
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 7823797e31d2..f0cec673583c 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -465,6 +465,10 @@ struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig)
> >       data_race(memcpy(new, orig, sizeof(*new)));
> >       vma_lock_init(new);
> >       INIT_LIST_HEAD(&new->anon_vma_chain);
> > +#ifdef CONFIG_PER_VMA_LOCK
> > +     /* vma is not locked, can't use vma_mark_detached() */
> > +     new->detached = true;
> > +#endif
> >       vma_numab_state_init(new);
> >       dup_anon_vma_name(orig, new);
> >
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 209885a4134f..d0197a0c0996 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -6279,7 +6279,7 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
> >               goto inval;
> >
> >       /* Check if the VMA got isolated after we found it */
> > -     if (vma->detached) {
> > +     if (is_vma_detached(vma)) {
> >               vma_end_read(vma);
> >               count_vm_vma_lock_event(VMA_LOCK_MISS);
> >               /* The area was replaced with another one */
> > diff --git a/mm/vma.c b/mm/vma.c
> > index 8a454a7bbc80..73104d434567 100644
> > --- a/mm/vma.c
> > +++ b/mm/vma.c
> > @@ -295,7 +295,7 @@ static void vma_complete(struct vma_prepare *vp, struct vma_iterator *vmi,
> >
> >       if (vp->remove) {
> >  again:
> > -             vma_mark_detached(vp->remove, true);
> > +             vma_mark_detached(vp->remove);
> >               if (vp->file) {
> >                       uprobe_munmap(vp->remove, vp->remove->vm_start,
> >                                     vp->remove->vm_end);
> > @@ -1220,7 +1220,7 @@ static void reattach_vmas(struct ma_state *mas_detach)
> >
> >       mas_set(mas_detach, 0);
> >       mas_for_each(mas_detach, vma, ULONG_MAX)
> > -             vma_mark_detached(vma, false);
> > +             vma_mark_attached(vma);
> >
> >       __mt_destroy(mas_detach->tree);
> >  }
>
> This is on a subtle error handling code path, we should definitely do some
> careful checking of this (it might be nice to add some to the vma.c
> userland tests...)
>
> > @@ -1295,7 +1295,7 @@ static int vms_gather_munmap_vmas(struct vma_munmap_struct *vms,
> >               if (error)
> >                       goto munmap_gather_failed;
> >
> > -             vma_mark_detached(next, true);
> > +             vma_mark_detached(next);
> >               nrpages = vma_pages(next);
> >
> >               vms->nr_pages += nrpages;
> > diff --git a/mm/vma.h b/mm/vma.h
> > index 388d34748674..2e680f357ace 100644
> > --- a/mm/vma.h
> > +++ b/mm/vma.h
> > @@ -162,6 +162,7 @@ static inline int vma_iter_store_gfp(struct vma_iterator *vmi,
> >       if (unlikely(mas_is_err(&vmi->mas)))
> >               return -ENOMEM;
> >
> > +     vma_mark_attached(vma);
> >       return 0;
> >  }
> >
> > @@ -385,6 +386,7 @@ static inline void vma_iter_store(struct vma_iterator *vmi,
> >
> >       __mas_set_range(&vmi->mas, vma->vm_start, vma->vm_end - 1);
> >       mas_store_prealloc(&vmi->mas, vma);
> > +     vma_mark_attached(vma);
> >  }
> >
> >  static inline unsigned long vma_iter_addr(struct vma_iterator *vmi)
> > diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
> > index 11c2c38ca4e8..2fed366d20ef 100644
> > --- a/tools/testing/vma/vma_internal.h
> > +++ b/tools/testing/vma/vma_internal.h
> > @@ -414,13 +414,17 @@ static inline void vma_lock_init(struct vm_area_struct *vma)
> >       vma->vm_lock_seq = UINT_MAX;
> >  }
> >
> > +static inline void vma_mark_attached(struct vm_area_struct *vma)
> > +{
> > +     vma->detached = false;
> > +}
> > +
> >  static inline void vma_assert_write_locked(struct vm_area_struct *);
> > -static inline void vma_mark_detached(struct vm_area_struct *vma, bool detached)
> > +static inline void vma_mark_detached(struct vm_area_struct *vma)
> >  {
> >       /* When detaching vma should be write-locked */
> > -     if (detached)
> > -             vma_assert_write_locked(vma);
> > -     vma->detached = detached;
> > +     vma_assert_write_locked(vma);
> > +     vma->detached = true;
> >  }
> >
> >  extern const struct vm_operations_struct vma_dummy_vm_ops;
> > @@ -431,7 +435,8 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
> >       vma->vm_mm = mm;
> >       vma->vm_ops = &vma_dummy_vm_ops;
> >       INIT_LIST_HEAD(&vma->anon_vma_chain);
> > -     vma_mark_detached(vma, false);
> > +     /* vma is not locked, can't use vma_mark_detached() */
> > +     vma->detached = true;
> >       vma_lock_init(vma);
> >  }
> >
> > @@ -457,6 +462,8 @@ static inline struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig)
> >       memcpy(new, orig, sizeof(*new));
> >       vma_lock_init(new);
> >       INIT_LIST_HEAD(&new->anon_vma_chain);
> > +     /* vma is not locked, can't use vma_mark_detached() */
> > +     new->detached = true;
> >
> >       return new;
> >  }
> > --
> > 2.47.0.338.g60cca15819-goog
> >

Re: [PATCH v3 3/5] mm: mark vma as detached until it's added into vma tree

[Suren Baghdasaryan]

On Mon, Nov 18, 2024 at 8:23 AM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Mon, Nov 18, 2024 at 6:10 AM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > So, this causes VMAs which are already attached to be marked attached
> > again, and when the check added in "mm: make vma cache
> > SLAB_TYPESAFE_BY_RCU", which is:
> >
> > static inline void vma_mark_attached(struct vm_area_struct *vma)
> > {
> >         /* vma shoudn't be already attached */
> >         VM_BUG_ON_VMA(!vma->detached, vma); <-------- here
> >
> >         ...
> > }
> >
> > Is executed, it fails and asserts on boot, as per the below (I ran
> > addr2line and identified this as the cause).
> >
> > [    0.615256] vma ffff88810086e000 start 00007ffedf98e000 end 00007ffffffff000 mm ffff888101bf0000
> > [    0.615256] prot 8000000000000025 anon_vma ffff88810026c000 vm_ops 0000000000000000
> > [    0.615256] pgoff 7fffffffe file 0000000000000000 private_data 0000000000000000
> > [    0.615256] flags: 0x118173(read|write|mayread|maywrite|mayexec|growsdown|seqread|randread|account)
> > [    0.616232] Oops: invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
> > [    0.616416] CPU: 3 UID: 0 PID: 1 Comm: init Not tainted 6.12.0-rc6+ #9
> > [    0.616618] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.3-1-1 04/01/2014
> > [    0.616914] RIP: 0010:commit_merge+0x361/0x390
> > [    0.617059] Code: 28 e9 58 fd ff ff 49 39 44 24 10 72 c7 e9 81 fe ff ff 48 39 57 10 0f 82 1d ff ff ff e9 1c ff ff ff 48 89 c7 e8 70 0
> > [    0.617609] RSP: 0018:ffffc90000013a48 EFLAGS: 00010292
> > [    0.617778] RAX: 0000000000000138 RBX: ffffc90000013b68 RCX: 0000000000000000
> > [    0.617995] RDX: 0000000000000003 RSI: ffffc900000138d0 RDI: 0000000000000001
> > [    0.618209] RBP: 0000000000000000 R08: 00000000ffffdfff R09: ffffffff82b089a8
> > [    0.618429] R10: 0000000000000003 R11: 30203a7367616c66 R12: 0000000000000000
> > [    0.618653] R13: 0000000000000001 R14: 0000000000000000 R15: ffffc90000013a58
> > [    0.618846] FS:  0000000000000000(0000) GS:ffff888263d80000(0000) knlGS:0000000000000000
> > [    0.619041] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [    0.619186] CR2: 0000000000000000 CR3: 0000000101c74000 CR4: 0000000000750ef0
> > [    0.619357] PKRU: 55555554
> > [    0.619425] Call Trace:
> > [    0.619491]  <TASK>
> > [    0.619546]  ? __die_body.cold+0x19/0x2a
> > [    0.619644]  ? die+0x29/0x50
> > [    0.619719]  ? do_trap+0xc5/0x110
> > [    0.619808]  ? do_error_trap+0x60/0x80
> > [    0.619901]  ? commit_merge+0x361/0x390
> > [    0.619995]  ? exc_invalid_op+0x51/0x70
> > [    0.620092]  ? commit_merge+0x361/0x390
> > [    0.620185]  ? asm_exc_invalid_op+0x1a/0x20
> > [    0.620288]  ? commit_merge+0x361/0x390
> > [    0.620383]  ? commit_merge+0x360/0x390
> > [    0.620478]  vma_expand+0xd0/0x1a0
> > [    0.620563]  relocate_vma_down+0xe8/0x1e0
> > [    0.620664]  setup_arg_pages+0x1f6/0x360
> > [    0.620783]  load_elf_binary+0x37b/0x1720
> > [    0.620912]  ? __kernel_read+0x187/0x2e0
> > [    0.621038]  ? load_misc_binary+0x225/0x2f0
> > [    0.621173]  bprm_execve+0x22e/0x5b0
> > [    0.621288]  kernel_execve+0x10b/0x140
> > [    0.621406]  try_to_run_init_process+0xa/0x2e
> > [    0.621545]  ? __pfx_kernel_init+0x10/0x10
> > [    0.621675]  kernel_init+0xde/0x130
> > [    0.621796]  ret_from_fork+0x2c/0x50
> > [    0.621914]  ? __pfx_kernel_init+0x10/0x10
> > [    0.622046]  ret_from_fork_asm+0x1a/0x30
> > [    0.622174]  </TASK>
> > [    0.622248] Modules linked in:
> > [    0.622356] ---[ end trace 0000000000000000 ]---
> > [    0.622502] RIP: 0010:commit_merge+0x361/0x390
> > [    0.622643] Code: 28 e9 58 fd ff ff 49 39 44 24 10 72 c7 e9 81 fe ff ff 48 39 57 10 0f 82 1d ff ff ff e9 1c ff ff ff 48 89 c7 e8 70 0
> > [    0.623213] RSP: 0018:ffffc90000013a48 EFLAGS: 00010292
> > [    0.623381] RAX: 0000000000000138 RBX: ffffc90000013b68 RCX: 0000000000000000
> > [    0.623596] RDX: 0000000000000003 RSI: ffffc900000138d0 RDI: 0000000000000001
> > [    0.623825] RBP: 0000000000000000 R08: 00000000ffffdfff R09: ffffffff82b089a8
> > [    0.624045] R10: 0000000000000003 R11: 30203a7367616c66 R12: 0000000000000000
> > [    0.624268] R13: 0000000000000001 R14: 0000000000000000 R15: ffffc90000013a58
> > [    0.624484] FS:  0000000000000000(0000) GS:ffff888263d80000(0000) knlGS:0000000000000000
> > [    0.624746] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [    0.624926] CR2: 0000000000000000 CR3: 0000000101c74000 CR4: 0000000000750ef0
> > [    0.625149] PKRU: 55555554
> > [    0.625244] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> > [    0.625545] Kernel Offset: disabled
> > [    0.625658] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---
> >
> > If I add code to detach first in relocate_vma_down(), then
> > expand_downwards() has the same issue. It seems this code doesn't account
> > for such cases.
> >
> > If I add code to fix _this_ then a VMA merge triggers it and... I think
> > this is just fundamentally broken...
> >
> > There are cases where we change the size of an existing VMA and overwrite
> > stuff in the maple tree, this is normal, and we do it to an attached VMA.
> >
> > So actually perhaps... we should just drop this check altogether?
> >
> > My workarounds are essentially to mark detached immediately prior to
> > vma_iter_store() which seems to defeat the purpose :P
>
> I realized that this assertion was added at a later stage of the patch
> and I tested it using the same config that I use for performance
> testing, which did not have CONFIG_DEBUG_VM enabled. Sorry about that.
> I didn't realize we are modifying and reinserting the vma without
> marking it detached, however these cases are not an issue for vma
> reuse because we do not free the vma in the process. I think the
> following should work fine:
>
> static inline void vma_mark_attached(struct vm_area_struct *vma)
> {
>         /* If vma is write-locked then it's already attached */
>         if (down_write_trylock(&vma->vm_lock.lock)) {
>                 vma->detached = false;
>                 up_write(&vma->vm_lock.lock);
>         }
> }
>
> I'll think some more about edge cases and will post the new patchset
> with the fix.

I posted v4 with a different approach here to avoid write-locking the
vma. All we need here is a strict access ordering between
vma->detached and vm_mm/vm_start/vm_and:
When attaching a vma, vm_mm/vm_start/vm_end should be set before vma
is marked attached;
When lock_vma_under_rcu() is validating a vma, vma->detached should be
checked before vm_mm/vm_start/vm_end.
It's implemented and explained in more details at
https://lore.kernel.org/all/20241120000826.335387-5-surenb@google.com/

> Thanks for reviewing and testing, Lorenzo!
> Suren.
>
> >
> > On Sun, Nov 17, 2024 at 12:09:29AM -0800, Suren Baghdasaryan wrote:
> > > Current implementation does not set detached flag when a VMA is first
> > > allocated. This does not represent the real state of the VMA, which is
> > > detached until it is added into mm's VMA tree. Fix this by marking new
> > > VMAs as detached and resetting detached flag only after VMA is added
> > > into a tree.
> > > Introduce vma_mark_attached() to make the API more readable and to
> > > simplify possible future cleanup when vma->vm_mm might be used to
> > > indicate detached vma and vma_mark_attached() will need an additional
> > > mm parameter.
> > >
> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > ---
> > >  include/linux/mm.h               | 27 ++++++++++++++++++++-------
> > >  kernel/fork.c                    |  4 ++++
> > >  mm/memory.c                      |  2 +-
> > >  mm/vma.c                         |  6 +++---
> > >  mm/vma.h                         |  2 ++
> > >  tools/testing/vma/vma_internal.h | 17 ++++++++++++-----
> > >  6 files changed, 42 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > index 737c003b0a1e..dd1b6190df28 100644
> > > --- a/include/linux/mm.h
> > > +++ b/include/linux/mm.h
> > > @@ -808,12 +808,21 @@ static inline void vma_assert_locked(struct vm_area_struct *vma)
> > >               vma_assert_write_locked(vma);
> > >  }
> > >
> > > -static inline void vma_mark_detached(struct vm_area_struct *vma, bool detached)
> > > +static inline void vma_mark_attached(struct vm_area_struct *vma)
> > > +{
> > > +     vma->detached = false;
> > > +}
> >
> > We should definitely add the
> >
> >         VM_BUG_ON_VMA(!vma->detached, vma);
> >
> > Check that is added in "mm: make vma cache SLAB_TYPESAFE_BY_RCU" here
> > instead, if we want it.
> >
> > But as per above I'm not sure we do...
> >
> > > +
> > > +static inline void vma_mark_detached(struct vm_area_struct *vma)
> > >  {
> > >       /* When detaching vma should be write-locked */
> > > -     if (detached)
> > > -             vma_assert_write_locked(vma);
> > > -     vma->detached = detached;
> > > +     vma_assert_write_locked(vma);
> > > +     vma->detached = true;
> > > +}
> >
> > Do we want to assert it was attached before? Then again given the attached
> > assert probably not :>)
> >
> > > +
> > > +static inline bool is_vma_detached(struct vm_area_struct *vma)
> > > +{
> > > +     return vma->detached;
> > >  }
> > >
> > >  static inline void release_fault_lock(struct vm_fault *vmf)
> > > @@ -844,8 +853,8 @@ static inline void vma_end_read(struct vm_area_struct *vma) {}
> > >  static inline void vma_start_write(struct vm_area_struct *vma) {}
> > >  static inline void vma_assert_write_locked(struct vm_area_struct *vma)
> > >               { mmap_assert_write_locked(vma->vm_mm); }
> > > -static inline void vma_mark_detached(struct vm_area_struct *vma,
> > > -                                  bool detached) {}
> > > +static inline void vma_mark_attached(struct vm_area_struct *vma) {}
> > > +static inline void vma_mark_detached(struct vm_area_struct *vma) {}
> > >
> > >  static inline struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
> > >               unsigned long address)
> > > @@ -878,7 +887,10 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
> > >       vma->vm_mm = mm;
> > >       vma->vm_ops = &vma_dummy_vm_ops;
> > >       INIT_LIST_HEAD(&vma->anon_vma_chain);
> > > -     vma_mark_detached(vma, false);
> > > +#ifdef CONFIG_PER_VMA_LOCK
> > > +     /* vma is not locked, can't use vma_mark_detached() */
> > > +     vma->detached = true;
> > > +#endif
> > >       vma_numab_state_init(vma);
> > >       vma_lock_init(vma);
> > >  }
> > > @@ -1073,6 +1085,7 @@ static inline int vma_iter_bulk_store(struct vma_iterator *vmi,
> > >       if (unlikely(mas_is_err(&vmi->mas)))
> > >               return -ENOMEM;
> > >
> > > +     vma_mark_attached(vma);
> > >       return 0;
> > >  }
> > >
> > > diff --git a/kernel/fork.c b/kernel/fork.c
> > > index 7823797e31d2..f0cec673583c 100644
> > > --- a/kernel/fork.c
> > > +++ b/kernel/fork.c
> > > @@ -465,6 +465,10 @@ struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig)
> > >       data_race(memcpy(new, orig, sizeof(*new)));
> > >       vma_lock_init(new);
> > >       INIT_LIST_HEAD(&new->anon_vma_chain);
> > > +#ifdef CONFIG_PER_VMA_LOCK
> > > +     /* vma is not locked, can't use vma_mark_detached() */
> > > +     new->detached = true;
> > > +#endif
> > >       vma_numab_state_init(new);
> > >       dup_anon_vma_name(orig, new);
> > >
> > > diff --git a/mm/memory.c b/mm/memory.c
> > > index 209885a4134f..d0197a0c0996 100644
> > > --- a/mm/memory.c
> > > +++ b/mm/memory.c
> > > @@ -6279,7 +6279,7 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
> > >               goto inval;
> > >
> > >       /* Check if the VMA got isolated after we found it */
> > > -     if (vma->detached) {
> > > +     if (is_vma_detached(vma)) {
> > >               vma_end_read(vma);
> > >               count_vm_vma_lock_event(VMA_LOCK_MISS);
> > >               /* The area was replaced with another one */
> > > diff --git a/mm/vma.c b/mm/vma.c
> > > index 8a454a7bbc80..73104d434567 100644
> > > --- a/mm/vma.c
> > > +++ b/mm/vma.c
> > > @@ -295,7 +295,7 @@ static void vma_complete(struct vma_prepare *vp, struct vma_iterator *vmi,
> > >
> > >       if (vp->remove) {
> > >  again:
> > > -             vma_mark_detached(vp->remove, true);
> > > +             vma_mark_detached(vp->remove);
> > >               if (vp->file) {
> > >                       uprobe_munmap(vp->remove, vp->remove->vm_start,
> > >                                     vp->remove->vm_end);
> > > @@ -1220,7 +1220,7 @@ static void reattach_vmas(struct ma_state *mas_detach)
> > >
> > >       mas_set(mas_detach, 0);
> > >       mas_for_each(mas_detach, vma, ULONG_MAX)
> > > -             vma_mark_detached(vma, false);
> > > +             vma_mark_attached(vma);
> > >
> > >       __mt_destroy(mas_detach->tree);
> > >  }
> >
> > This is on a subtle error handling code path, we should definitely do some
> > careful checking of this (it might be nice to add some to the vma.c
> > userland tests...)
> >
> > > @@ -1295,7 +1295,7 @@ static int vms_gather_munmap_vmas(struct vma_munmap_struct *vms,
> > >               if (error)
> > >                       goto munmap_gather_failed;
> > >
> > > -             vma_mark_detached(next, true);
> > > +             vma_mark_detached(next);
> > >               nrpages = vma_pages(next);
> > >
> > >               vms->nr_pages += nrpages;
> > > diff --git a/mm/vma.h b/mm/vma.h
> > > index 388d34748674..2e680f357ace 100644
> > > --- a/mm/vma.h
> > > +++ b/mm/vma.h
> > > @@ -162,6 +162,7 @@ static inline int vma_iter_store_gfp(struct vma_iterator *vmi,
> > >       if (unlikely(mas_is_err(&vmi->mas)))
> > >               return -ENOMEM;
> > >
> > > +     vma_mark_attached(vma);
> > >       return 0;
> > >  }
> > >
> > > @@ -385,6 +386,7 @@ static inline void vma_iter_store(struct vma_iterator *vmi,
> > >
> > >       __mas_set_range(&vmi->mas, vma->vm_start, vma->vm_end - 1);
> > >       mas_store_prealloc(&vmi->mas, vma);
> > > +     vma_mark_attached(vma);
> > >  }
> > >
> > >  static inline unsigned long vma_iter_addr(struct vma_iterator *vmi)
> > > diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
> > > index 11c2c38ca4e8..2fed366d20ef 100644
> > > --- a/tools/testing/vma/vma_internal.h
> > > +++ b/tools/testing/vma/vma_internal.h
> > > @@ -414,13 +414,17 @@ static inline void vma_lock_init(struct vm_area_struct *vma)
> > >       vma->vm_lock_seq = UINT_MAX;
> > >  }
> > >
> > > +static inline void vma_mark_attached(struct vm_area_struct *vma)
> > > +{
> > > +     vma->detached = false;
> > > +}
> > > +
> > >  static inline void vma_assert_write_locked(struct vm_area_struct *);
> > > -static inline void vma_mark_detached(struct vm_area_struct *vma, bool detached)
> > > +static inline void vma_mark_detached(struct vm_area_struct *vma)
> > >  {
> > >       /* When detaching vma should be write-locked */
> > > -     if (detached)
> > > -             vma_assert_write_locked(vma);
> > > -     vma->detached = detached;
> > > +     vma_assert_write_locked(vma);
> > > +     vma->detached = true;
> > >  }
> > >
> > >  extern const struct vm_operations_struct vma_dummy_vm_ops;
> > > @@ -431,7 +435,8 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
> > >       vma->vm_mm = mm;
> > >       vma->vm_ops = &vma_dummy_vm_ops;
> > >       INIT_LIST_HEAD(&vma->anon_vma_chain);
> > > -     vma_mark_detached(vma, false);
> > > +     /* vma is not locked, can't use vma_mark_detached() */
> > > +     vma->detached = true;
> > >       vma_lock_init(vma);
> > >  }
> > >
> > > @@ -457,6 +462,8 @@ static inline struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig)
> > >       memcpy(new, orig, sizeof(*new));
> > >       vma_lock_init(new);
> > >       INIT_LIST_HEAD(&new->anon_vma_chain);
> > > +     /* vma is not locked, can't use vma_mark_detached() */
> > > +     new->detached = true;
> > >
> > >       return new;
> > >  }
> > > --
> > > 2.47.0.338.g60cca15819-goog
> > >

[PATCH v3 4/5] mm: make vma cache SLAB_TYPESAFE_BY_RCU

[Suren Baghdasaryan]

To enable SLAB_TYPESAFE_BY_RCU for vma cache we need to ensure that
object reuse before RCU grace period is over will be detected inside
lock_vma_under_rcu().
lock_vma_under_rcu() enters RCU read section, finds the vma at the
given address, locks the vma and checks if it got detached or remapped
to cover a different address range. These last checks are there
to ensure that the vma was not modified after we found it but before
locking it.
vma reuse introduces several new possibilities:
1. vma can be reused after it was found but before it is locked;
2. vma can be reused and reinitialized (including changing its vm_mm)
while being locked in vma_start_read();
3. vma can be reused and reinitialized after it was found but before
it is locked, then attached at a new address or to a new mm while being
read-locked;
For case #1 current checks will help detecting cases when:
- vma was reused but not yet added into the tree (detached check)
- vma was reused at a different address range (address check);
We are missing the check for vm_mm to ensure the reused vma was not
attached to a different mm. This patch adds the missing check.
For case #2, we pass mm to vma_start_read() to prevent access to
unstable vma->vm_mm.
For case #3, we write-lock the vma in vma_mark_attached(), ensuring that
vma does not get re-attached while read-locked by a user of the vma
before it was recycled.
This write-locking should not cause performance issues because contention
during vma_mark_attached() can happen only in the rare vma reuse case.
Even when this happens, it's the slowpath (write-lock) which will be
waiting, not the page fault path.
After these provisions, SLAB_TYPESAFE_BY_RCU is added to vm_area_cachep.
This will facilitate vm_area_struct reuse and will minimize the number
of call_rcu() calls.
Adding a freeptr_t into vm_area_struct (unioned with vm_start/vm_end)
could be used to avoids bloating the structure, however currently
custom free pointers are not supported in combination with a ctor
(see the comment for kmem_cache_args.freeptr_offset).

Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
 include/linux/mm.h               | 48 ++++++++++++++++++++++++-----
 include/linux/mm_types.h         | 13 +++-----
 kernel/fork.c                    | 53 +++++++++++++++++++-------------
 mm/memory.c                      |  7 +++--
 mm/vma.c                         |  2 +-
 tools/testing/vma/vma_internal.h |  7 +++--
 6 files changed, 86 insertions(+), 44 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index dd1b6190df28..d8e10e1e34ad 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -257,7 +257,7 @@ struct vm_area_struct *vm_area_alloc(struct mm_struct *);
 struct vm_area_struct *vm_area_dup(struct vm_area_struct *);
 void vm_area_free(struct vm_area_struct *);
 /* Use only if VMA has no other users */
-void __vm_area_free(struct vm_area_struct *vma);
+void vm_area_free_unreachable(struct vm_area_struct *vma);
 
 #ifndef CONFIG_MMU
 extern struct rb_root nommu_region_tree;
@@ -690,12 +690,32 @@ static inline void vma_lock_init(struct vm_area_struct *vma)
 	vma->vm_lock_seq = UINT_MAX;
 }
 
+#define VMA_BEFORE_LOCK		offsetof(struct vm_area_struct, vm_lock)
+#define VMA_LOCK_END(vma)	\
+	(((void *)(vma)) + offsetofend(struct vm_area_struct, vm_lock))
+#define VMA_AFTER_LOCK		\
+	(sizeof(struct vm_area_struct) - offsetofend(struct vm_area_struct, vm_lock))
+
+static inline void vma_clear(struct vm_area_struct *vma)
+{
+	/* Preserve vma->vm_lock */
+	memset(vma, 0, VMA_BEFORE_LOCK);
+	memset(VMA_LOCK_END(vma), 0, VMA_AFTER_LOCK);
+}
+
+static inline void vma_copy(struct vm_area_struct *new, struct vm_area_struct *orig)
+{
+	/* Preserve vma->vm_lock */
+	data_race(memcpy(new, orig, VMA_BEFORE_LOCK));
+	data_race(memcpy(VMA_LOCK_END(new), VMA_LOCK_END(orig), VMA_AFTER_LOCK));
+}
+
 /*
  * Try to read-lock a vma. The function is allowed to occasionally yield false
  * locked result to avoid performance overhead, in which case we fall back to
  * using mmap_lock. The function should never yield false unlocked result.
  */
-static inline bool vma_start_read(struct vm_area_struct *vma)
+static inline bool vma_start_read(struct mm_struct *mm, struct vm_area_struct *vma)
 {
 	/*
 	 * Check before locking. A race might cause false locked result.
@@ -704,7 +724,7 @@ static inline bool vma_start_read(struct vm_area_struct *vma)
 	 * we don't rely on for anything - the mm_lock_seq read against which we
 	 * need ordering is below.
 	 */
-	if (READ_ONCE(vma->vm_lock_seq) == READ_ONCE(vma->vm_mm->mm_lock_seq.sequence))
+	if (READ_ONCE(vma->vm_lock_seq) == READ_ONCE(mm->mm_lock_seq.sequence))
 		return false;
 
 	if (unlikely(down_read_trylock(&vma->vm_lock.lock) == 0))
@@ -721,7 +741,7 @@ static inline bool vma_start_read(struct vm_area_struct *vma)
 	 * after it has been unlocked.
 	 * This pairs with RELEASE semantics in vma_end_write_all().
 	 */
-	if (unlikely(vma->vm_lock_seq == raw_read_seqcount(&vma->vm_mm->mm_lock_seq))) {
+	if (unlikely(vma->vm_lock_seq == raw_read_seqcount(&mm->mm_lock_seq))) {
 		up_read(&vma->vm_lock.lock);
 		return false;
 	}
@@ -810,7 +830,18 @@ static inline void vma_assert_locked(struct vm_area_struct *vma)
 
 static inline void vma_mark_attached(struct vm_area_struct *vma)
 {
+	/* vma shoudn't be already attached */
+	VM_BUG_ON_VMA(!vma->detached, vma);
+
+	/*
+	 * Lock here can be contended only if the vma got reused after
+	 * lock_vma_under_rcu() found it but before it had a chance to
+	 * read-lock it. Write-locking the vma guarantees that the vma
+	 * won't be attached until all its old users are out.
+	 */
+	down_write(&vma->vm_lock.lock);
 	vma->detached = false;
+	up_write(&vma->vm_lock.lock);
 }
 
 static inline void vma_mark_detached(struct vm_area_struct *vma)
@@ -847,7 +878,11 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
 #else /* CONFIG_PER_VMA_LOCK */
 
 static inline void vma_lock_init(struct vm_area_struct *vma) {}
-static inline bool vma_start_read(struct vm_area_struct *vma)
+static inline void vma_clear(struct vm_area_struct *vma)
+		{ memset(vma, 0, sizeof(*vma)); }
+static inline void vma_copy(struct vm_area_struct *new, struct vm_area_struct *orig)
+		{ data_race(memcpy(new, orig, sizeof(*new))); }
+static inline bool vma_start_read(struct mm_struct *mm, struct vm_area_struct *vma)
 		{ return false; }
 static inline void vma_end_read(struct vm_area_struct *vma) {}
 static inline void vma_start_write(struct vm_area_struct *vma) {}
@@ -883,7 +918,7 @@ extern const struct vm_operations_struct vma_dummy_vm_ops;
 
 static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
 {
-	memset(vma, 0, sizeof(*vma));
+	vma_clear(vma);
 	vma->vm_mm = mm;
 	vma->vm_ops = &vma_dummy_vm_ops;
 	INIT_LIST_HEAD(&vma->anon_vma_chain);
@@ -892,7 +927,6 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
 	vma->detached = true;
 #endif
 	vma_numab_state_init(vma);
-	vma_lock_init(vma);
 }
 
 /* Use when VMA is not part of the VMA tree and needs no locking */
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 5c4bfdcfac72..8f6b0c935c2b 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -667,15 +667,10 @@ struct vma_numab_state {
 struct vm_area_struct {
 	/* The first cache line has the info for VMA tree walking. */
 
-	union {
-		struct {
-			/* VMA covers [vm_start; vm_end) addresses within mm */
-			unsigned long vm_start;
-			unsigned long vm_end;
-		};
-#ifdef CONFIG_PER_VMA_LOCK
-		struct rcu_head vm_rcu;	/* Used for deferred freeing. */
-#endif
+	struct {
+		/* VMA covers [vm_start; vm_end) addresses within mm */
+		unsigned long vm_start;
+		unsigned long vm_end;
 	};
 
 	/*
diff --git a/kernel/fork.c b/kernel/fork.c
index f0cec673583c..76c68b041f8a 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -436,6 +436,11 @@ static struct kmem_cache *vm_area_cachep;
 /* SLAB cache for mm_struct structures (tsk->mm) */
 static struct kmem_cache *mm_cachep;
 
+static void vm_area_ctor(void *data)
+{
+	vma_lock_init(data);
+}
+
 struct vm_area_struct *vm_area_alloc(struct mm_struct *mm)
 {
 	struct vm_area_struct *vma;
@@ -462,8 +467,7 @@ struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig)
 	 * orig->shared.rb may be modified concurrently, but the clone
 	 * will be reinitialized.
 	 */
-	data_race(memcpy(new, orig, sizeof(*new)));
-	vma_lock_init(new);
+	vma_copy(new, orig);
 	INIT_LIST_HEAD(&new->anon_vma_chain);
 #ifdef CONFIG_PER_VMA_LOCK
 	/* vma is not locked, can't use vma_mark_detached() */
@@ -475,32 +479,37 @@ struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig)
 	return new;
 }
 
-void __vm_area_free(struct vm_area_struct *vma)
+static void __vm_area_free(struct vm_area_struct *vma, bool unreachable)
 {
+#ifdef CONFIG_PER_VMA_LOCK
+	/*
+	 * With SLAB_TYPESAFE_BY_RCU, vma can be reused and we need
+	 * vma->detached to be set before vma is returned into the cache.
+	 * This way reused object won't be used by readers until it's
+	 * initialized and reattached.
+	 * If vma is unreachable, there can be no other users and we
+	 * can set vma->detached directly with no risk of a race.
+	 * If vma is reachable, then it should have been already detached
+	 * under vma write-lock or it was never attached.
+	 */
+	if (unreachable)
+		vma->detached = true;
+	else
+		VM_BUG_ON_VMA(!is_vma_detached(vma), vma);
+#endif
 	vma_numab_state_free(vma);
 	free_anon_vma_name(vma);
 	kmem_cache_free(vm_area_cachep, vma);
 }
 
-#ifdef CONFIG_PER_VMA_LOCK
-static void vm_area_free_rcu_cb(struct rcu_head *head)
+void vm_area_free(struct vm_area_struct *vma)
 {
-	struct vm_area_struct *vma = container_of(head, struct vm_area_struct,
-						  vm_rcu);
-
-	/* The vma should not be locked while being destroyed. */
-	VM_BUG_ON_VMA(rwsem_is_locked(&vma->vm_lock.lock), vma);
-	__vm_area_free(vma);
+	__vm_area_free(vma, false);
 }
-#endif
 
-void vm_area_free(struct vm_area_struct *vma)
+void vm_area_free_unreachable(struct vm_area_struct *vma)
 {
-#ifdef CONFIG_PER_VMA_LOCK
-	call_rcu(&vma->vm_rcu, vm_area_free_rcu_cb);
-#else
-	__vm_area_free(vma);
-#endif
+	__vm_area_free(vma, true);
 }
 
 static void account_kernel_stack(struct task_struct *tsk, int account)
@@ -3135,9 +3144,11 @@ void __init proc_caches_init(void)
 			sizeof(struct fs_struct), 0,
 			SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_ACCOUNT,
 			NULL);
-	vm_area_cachep = KMEM_CACHE(vm_area_struct,
-			SLAB_HWCACHE_ALIGN|SLAB_NO_MERGE|SLAB_PANIC|
-			SLAB_ACCOUNT);
+	vm_area_cachep = kmem_cache_create("vm_area_struct",
+			sizeof(struct vm_area_struct), 0,
+			SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_TYPESAFE_BY_RCU|
+			SLAB_ACCOUNT, vm_area_ctor);
+
 	mmap_init();
 	nsproxy_cache_init();
 }
diff --git a/mm/memory.c b/mm/memory.c
index d0197a0c0996..c8a3e820ed66 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -6275,7 +6275,7 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
 	if (!vma)
 		goto inval;
 
-	if (!vma_start_read(vma))
+	if (!vma_start_read(mm, vma))
 		goto inval;
 
 	/* Check if the VMA got isolated after we found it */
@@ -6292,8 +6292,9 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
 	 * fields are accessible for RCU readers.
 	 */
 
-	/* Check since vm_start/vm_end might change before we lock the VMA */
-	if (unlikely(address < vma->vm_start || address >= vma->vm_end))
+	/* Check since vm_mm/vm_start/vm_end might change before we lock the VMA */
+	if (unlikely(vma->vm_mm != mm ||
+		     address < vma->vm_start || address >= vma->vm_end))
 		goto inval_end_read;
 
 	rcu_read_unlock();
diff --git a/mm/vma.c b/mm/vma.c
index 73104d434567..050b83df3df2 100644
--- a/mm/vma.c
+++ b/mm/vma.c
@@ -382,7 +382,7 @@ void remove_vma(struct vm_area_struct *vma, bool unreachable)
 		fput(vma->vm_file);
 	mpol_put(vma_policy(vma));
 	if (unreachable)
-		__vm_area_free(vma);
+		vm_area_free_unreachable(vma);
 	else
 		vm_area_free(vma);
 }
diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
index 2fed366d20ef..fd668d6cafc0 100644
--- a/tools/testing/vma/vma_internal.h
+++ b/tools/testing/vma/vma_internal.h
@@ -632,14 +632,15 @@ static inline void mpol_put(struct mempolicy *)
 {
 }
 
-static inline void __vm_area_free(struct vm_area_struct *vma)
+static inline void vm_area_free(struct vm_area_struct *vma)
 {
 	free(vma);
 }
 
-static inline void vm_area_free(struct vm_area_struct *vma)
+static inline void vm_area_free_unreachable(struct vm_area_struct *vma)
 {
-	__vm_area_free(vma);
+	vma->detached = true;
+	vm_area_free(vma);
 }
 
 static inline void lru_add_drain(void)
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v3 4/5] mm: make vma cache SLAB_TYPESAFE_BY_RCU

[Lorenzo Stoakes]

On Sun, Nov 17, 2024 at 12:09:30AM -0800, Suren Baghdasaryan wrote:
> To enable SLAB_TYPESAFE_BY_RCU for vma cache we need to ensure that
> object reuse before RCU grace period is over will be detected inside
> lock_vma_under_rcu().
> lock_vma_under_rcu() enters RCU read section, finds the vma at the
> given address, locks the vma and checks if it got detached or remapped
> to cover a different address range. These last checks are there
> to ensure that the vma was not modified after we found it but before
> locking it.
> vma reuse introduces several new possibilities:
> 1. vma can be reused after it was found but before it is locked;
> 2. vma can be reused and reinitialized (including changing its vm_mm)
> while being locked in vma_start_read();
> 3. vma can be reused and reinitialized after it was found but before
> it is locked, then attached at a new address or to a new mm while being
> read-locked;
> For case #1 current checks will help detecting cases when:
> - vma was reused but not yet added into the tree (detached check)
> - vma was reused at a different address range (address check);
> We are missing the check for vm_mm to ensure the reused vma was not
> attached to a different mm. This patch adds the missing check.
> For case #2, we pass mm to vma_start_read() to prevent access to
> unstable vma->vm_mm.
> For case #3, we write-lock the vma in vma_mark_attached(), ensuring that
> vma does not get re-attached while read-locked by a user of the vma
> before it was recycled.
> This write-locking should not cause performance issues because contention
> during vma_mark_attached() can happen only in the rare vma reuse case.
> Even when this happens, it's the slowpath (write-lock) which will be
> waiting, not the page fault path.
> After these provisions, SLAB_TYPESAFE_BY_RCU is added to vm_area_cachep.
> This will facilitate vm_area_struct reuse and will minimize the number
> of call_rcu() calls.
> Adding a freeptr_t into vm_area_struct (unioned with vm_start/vm_end)
> could be used to avoids bloating the structure, however currently
> custom free pointers are not supported in combination with a ctor
> (see the comment for kmem_cache_args.freeptr_offset).
>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>

I've stayed out of this discussion as my slab knowledge is far less than
others (e.g. Vlastimil), but I shazam'd this series today and the kernel
isn't booting on my qemu setup, and I bisected it to this commit, and an
addr2line decode tells me this is:

static inline void vma_mark_attached(struct vm_area_struct *vma)
{
	/* vma shoudn't be already attached */
	VM_BUG_ON_VMA(!vma->detached, vma); <-------- here

	...
}

And if I go back to prior commits, this triggers too, since "mm: mark vma
as detached until it's added into vma tree".

I don't think this check should be added in this commit at any rate, it
should be added in "mm: mark vma as detached until it's added into vma
tree".

I will go to that commit to continue this investigation, but we should
definitely move that check over there.

> ---
>  include/linux/mm.h               | 48 ++++++++++++++++++++++++-----
>  include/linux/mm_types.h         | 13 +++-----
>  kernel/fork.c                    | 53 +++++++++++++++++++-------------
>  mm/memory.c                      |  7 +++--
>  mm/vma.c                         |  2 +-
>  tools/testing/vma/vma_internal.h |  7 +++--
>  6 files changed, 86 insertions(+), 44 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index dd1b6190df28..d8e10e1e34ad 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -257,7 +257,7 @@ struct vm_area_struct *vm_area_alloc(struct mm_struct *);
>  struct vm_area_struct *vm_area_dup(struct vm_area_struct *);
>  void vm_area_free(struct vm_area_struct *);
>  /* Use only if VMA has no other users */
> -void __vm_area_free(struct vm_area_struct *vma);
> +void vm_area_free_unreachable(struct vm_area_struct *vma);
>
>  #ifndef CONFIG_MMU
>  extern struct rb_root nommu_region_tree;
> @@ -690,12 +690,32 @@ static inline void vma_lock_init(struct vm_area_struct *vma)
>  	vma->vm_lock_seq = UINT_MAX;
>  }
>
> +#define VMA_BEFORE_LOCK		offsetof(struct vm_area_struct, vm_lock)
> +#define VMA_LOCK_END(vma)	\
> +	(((void *)(vma)) + offsetofend(struct vm_area_struct, vm_lock))
> +#define VMA_AFTER_LOCK		\
> +	(sizeof(struct vm_area_struct) - offsetofend(struct vm_area_struct, vm_lock))
> +
> +static inline void vma_clear(struct vm_area_struct *vma)
> +{
> +	/* Preserve vma->vm_lock */
> +	memset(vma, 0, VMA_BEFORE_LOCK);
> +	memset(VMA_LOCK_END(vma), 0, VMA_AFTER_LOCK);
> +}
> +
> +static inline void vma_copy(struct vm_area_struct *new, struct vm_area_struct *orig)
> +{
> +	/* Preserve vma->vm_lock */
> +	data_race(memcpy(new, orig, VMA_BEFORE_LOCK));
> +	data_race(memcpy(VMA_LOCK_END(new), VMA_LOCK_END(orig), VMA_AFTER_LOCK));
> +}
> +
>  /*
>   * Try to read-lock a vma. The function is allowed to occasionally yield false
>   * locked result to avoid performance overhead, in which case we fall back to
>   * using mmap_lock. The function should never yield false unlocked result.
>   */
> -static inline bool vma_start_read(struct vm_area_struct *vma)
> +static inline bool vma_start_read(struct mm_struct *mm, struct vm_area_struct *vma)
>  {
>  	/*
>  	 * Check before locking. A race might cause false locked result.
> @@ -704,7 +724,7 @@ static inline bool vma_start_read(struct vm_area_struct *vma)
>  	 * we don't rely on for anything - the mm_lock_seq read against which we
>  	 * need ordering is below.
>  	 */
> -	if (READ_ONCE(vma->vm_lock_seq) == READ_ONCE(vma->vm_mm->mm_lock_seq.sequence))
> +	if (READ_ONCE(vma->vm_lock_seq) == READ_ONCE(mm->mm_lock_seq.sequence))
>  		return false;
>
>  	if (unlikely(down_read_trylock(&vma->vm_lock.lock) == 0))
> @@ -721,7 +741,7 @@ static inline bool vma_start_read(struct vm_area_struct *vma)
>  	 * after it has been unlocked.
>  	 * This pairs with RELEASE semantics in vma_end_write_all().
>  	 */
> -	if (unlikely(vma->vm_lock_seq == raw_read_seqcount(&vma->vm_mm->mm_lock_seq))) {
> +	if (unlikely(vma->vm_lock_seq == raw_read_seqcount(&mm->mm_lock_seq))) {
>  		up_read(&vma->vm_lock.lock);
>  		return false;
>  	}
> @@ -810,7 +830,18 @@ static inline void vma_assert_locked(struct vm_area_struct *vma)
>
>  static inline void vma_mark_attached(struct vm_area_struct *vma)
>  {
> +	/* vma shoudn't be already attached */
> +	VM_BUG_ON_VMA(!vma->detached, vma);
> +
> +	/*
> +	 * Lock here can be contended only if the vma got reused after
> +	 * lock_vma_under_rcu() found it but before it had a chance to
> +	 * read-lock it. Write-locking the vma guarantees that the vma
> +	 * won't be attached until all its old users are out.
> +	 */
> +	down_write(&vma->vm_lock.lock);
>  	vma->detached = false;
> +	up_write(&vma->vm_lock.lock);
>  }
>
>  static inline void vma_mark_detached(struct vm_area_struct *vma)
> @@ -847,7 +878,11 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
>  #else /* CONFIG_PER_VMA_LOCK */
>
>  static inline void vma_lock_init(struct vm_area_struct *vma) {}
> -static inline bool vma_start_read(struct vm_area_struct *vma)
> +static inline void vma_clear(struct vm_area_struct *vma)
> +		{ memset(vma, 0, sizeof(*vma)); }
> +static inline void vma_copy(struct vm_area_struct *new, struct vm_area_struct *orig)
> +		{ data_race(memcpy(new, orig, sizeof(*new))); }
> +static inline bool vma_start_read(struct mm_struct *mm, struct vm_area_struct *vma)
>  		{ return false; }
>  static inline void vma_end_read(struct vm_area_struct *vma) {}
>  static inline void vma_start_write(struct vm_area_struct *vma) {}
> @@ -883,7 +918,7 @@ extern const struct vm_operations_struct vma_dummy_vm_ops;
>
>  static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
>  {
> -	memset(vma, 0, sizeof(*vma));
> +	vma_clear(vma);
>  	vma->vm_mm = mm;
>  	vma->vm_ops = &vma_dummy_vm_ops;
>  	INIT_LIST_HEAD(&vma->anon_vma_chain);
> @@ -892,7 +927,6 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
>  	vma->detached = true;
>  #endif
>  	vma_numab_state_init(vma);
> -	vma_lock_init(vma);
>  }
>
>  /* Use when VMA is not part of the VMA tree and needs no locking */
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 5c4bfdcfac72..8f6b0c935c2b 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -667,15 +667,10 @@ struct vma_numab_state {
>  struct vm_area_struct {
>  	/* The first cache line has the info for VMA tree walking. */
>
> -	union {
> -		struct {
> -			/* VMA covers [vm_start; vm_end) addresses within mm */
> -			unsigned long vm_start;
> -			unsigned long vm_end;
> -		};
> -#ifdef CONFIG_PER_VMA_LOCK
> -		struct rcu_head vm_rcu;	/* Used for deferred freeing. */
> -#endif
> +	struct {
> +		/* VMA covers [vm_start; vm_end) addresses within mm */
> +		unsigned long vm_start;
> +		unsigned long vm_end;
>  	};
>
>  	/*
> diff --git a/kernel/fork.c b/kernel/fork.c
> index f0cec673583c..76c68b041f8a 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -436,6 +436,11 @@ static struct kmem_cache *vm_area_cachep;
>  /* SLAB cache for mm_struct structures (tsk->mm) */
>  static struct kmem_cache *mm_cachep;
>
> +static void vm_area_ctor(void *data)
> +{
> +	vma_lock_init(data);
> +}
> +
>  struct vm_area_struct *vm_area_alloc(struct mm_struct *mm)
>  {
>  	struct vm_area_struct *vma;
> @@ -462,8 +467,7 @@ struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig)
>  	 * orig->shared.rb may be modified concurrently, but the clone
>  	 * will be reinitialized.
>  	 */
> -	data_race(memcpy(new, orig, sizeof(*new)));
> -	vma_lock_init(new);
> +	vma_copy(new, orig);
>  	INIT_LIST_HEAD(&new->anon_vma_chain);
>  #ifdef CONFIG_PER_VMA_LOCK
>  	/* vma is not locked, can't use vma_mark_detached() */
> @@ -475,32 +479,37 @@ struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig)
>  	return new;
>  }
>
> -void __vm_area_free(struct vm_area_struct *vma)
> +static void __vm_area_free(struct vm_area_struct *vma, bool unreachable)
>  {
> +#ifdef CONFIG_PER_VMA_LOCK
> +	/*
> +	 * With SLAB_TYPESAFE_BY_RCU, vma can be reused and we need
> +	 * vma->detached to be set before vma is returned into the cache.
> +	 * This way reused object won't be used by readers until it's
> +	 * initialized and reattached.
> +	 * If vma is unreachable, there can be no other users and we
> +	 * can set vma->detached directly with no risk of a race.
> +	 * If vma is reachable, then it should have been already detached
> +	 * under vma write-lock or it was never attached.
> +	 */
> +	if (unreachable)
> +		vma->detached = true;
> +	else
> +		VM_BUG_ON_VMA(!is_vma_detached(vma), vma);
> +#endif
>  	vma_numab_state_free(vma);
>  	free_anon_vma_name(vma);
>  	kmem_cache_free(vm_area_cachep, vma);
>  }
>
> -#ifdef CONFIG_PER_VMA_LOCK
> -static void vm_area_free_rcu_cb(struct rcu_head *head)
> +void vm_area_free(struct vm_area_struct *vma)
>  {
> -	struct vm_area_struct *vma = container_of(head, struct vm_area_struct,
> -						  vm_rcu);
> -
> -	/* The vma should not be locked while being destroyed. */
> -	VM_BUG_ON_VMA(rwsem_is_locked(&vma->vm_lock.lock), vma);
> -	__vm_area_free(vma);
> +	__vm_area_free(vma, false);
>  }
> -#endif
>
> -void vm_area_free(struct vm_area_struct *vma)
> +void vm_area_free_unreachable(struct vm_area_struct *vma)
>  {
> -#ifdef CONFIG_PER_VMA_LOCK
> -	call_rcu(&vma->vm_rcu, vm_area_free_rcu_cb);
> -#else
> -	__vm_area_free(vma);
> -#endif
> +	__vm_area_free(vma, true);
>  }
>
>  static void account_kernel_stack(struct task_struct *tsk, int account)
> @@ -3135,9 +3144,11 @@ void __init proc_caches_init(void)
>  			sizeof(struct fs_struct), 0,
>  			SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_ACCOUNT,
>  			NULL);
> -	vm_area_cachep = KMEM_CACHE(vm_area_struct,
> -			SLAB_HWCACHE_ALIGN|SLAB_NO_MERGE|SLAB_PANIC|
> -			SLAB_ACCOUNT);
> +	vm_area_cachep = kmem_cache_create("vm_area_struct",
> +			sizeof(struct vm_area_struct), 0,
> +			SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_TYPESAFE_BY_RCU|
> +			SLAB_ACCOUNT, vm_area_ctor);
> +
>  	mmap_init();
>  	nsproxy_cache_init();
>  }
> diff --git a/mm/memory.c b/mm/memory.c
> index d0197a0c0996..c8a3e820ed66 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -6275,7 +6275,7 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
>  	if (!vma)
>  		goto inval;
>
> -	if (!vma_start_read(vma))
> +	if (!vma_start_read(mm, vma))
>  		goto inval;
>
>  	/* Check if the VMA got isolated after we found it */
> @@ -6292,8 +6292,9 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
>  	 * fields are accessible for RCU readers.
>  	 */
>
> -	/* Check since vm_start/vm_end might change before we lock the VMA */
> -	if (unlikely(address < vma->vm_start || address >= vma->vm_end))
> +	/* Check since vm_mm/vm_start/vm_end might change before we lock the VMA */
> +	if (unlikely(vma->vm_mm != mm ||
> +		     address < vma->vm_start || address >= vma->vm_end))
>  		goto inval_end_read;
>
>  	rcu_read_unlock();
> diff --git a/mm/vma.c b/mm/vma.c
> index 73104d434567..050b83df3df2 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -382,7 +382,7 @@ void remove_vma(struct vm_area_struct *vma, bool unreachable)
>  		fput(vma->vm_file);
>  	mpol_put(vma_policy(vma));
>  	if (unreachable)
> -		__vm_area_free(vma);
> +		vm_area_free_unreachable(vma);
>  	else
>  		vm_area_free(vma);
>  }
> diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
> index 2fed366d20ef..fd668d6cafc0 100644
> --- a/tools/testing/vma/vma_internal.h
> +++ b/tools/testing/vma/vma_internal.h
> @@ -632,14 +632,15 @@ static inline void mpol_put(struct mempolicy *)
>  {
>  }
>
> -static inline void __vm_area_free(struct vm_area_struct *vma)
> +static inline void vm_area_free(struct vm_area_struct *vma)
>  {
>  	free(vma);
>  }
>
> -static inline void vm_area_free(struct vm_area_struct *vma)
> +static inline void vm_area_free_unreachable(struct vm_area_struct *vma)
>  {
> -	__vm_area_free(vma);
> +	vma->detached = true;
> +	vm_area_free(vma);
>  }
>
>  static inline void lru_add_drain(void)
> --
> 2.47.0.338.g60cca15819-goog
>

Re: [PATCH v3 4/5] mm: make vma cache SLAB_TYPESAFE_BY_RCU

[Suren Baghdasaryan]

On Mon, Nov 18, 2024 at 6:06 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Sun, Nov 17, 2024 at 12:09:30AM -0800, Suren Baghdasaryan wrote:
> > To enable SLAB_TYPESAFE_BY_RCU for vma cache we need to ensure that
> > object reuse before RCU grace period is over will be detected inside
> > lock_vma_under_rcu().
> > lock_vma_under_rcu() enters RCU read section, finds the vma at the
> > given address, locks the vma and checks if it got detached or remapped
> > to cover a different address range. These last checks are there
> > to ensure that the vma was not modified after we found it but before
> > locking it.
> > vma reuse introduces several new possibilities:
> > 1. vma can be reused after it was found but before it is locked;
> > 2. vma can be reused and reinitialized (including changing its vm_mm)
> > while being locked in vma_start_read();
> > 3. vma can be reused and reinitialized after it was found but before
> > it is locked, then attached at a new address or to a new mm while being
> > read-locked;
> > For case #1 current checks will help detecting cases when:
> > - vma was reused but not yet added into the tree (detached check)
> > - vma was reused at a different address range (address check);
> > We are missing the check for vm_mm to ensure the reused vma was not
> > attached to a different mm. This patch adds the missing check.
> > For case #2, we pass mm to vma_start_read() to prevent access to
> > unstable vma->vm_mm.
> > For case #3, we write-lock the vma in vma_mark_attached(), ensuring that
> > vma does not get re-attached while read-locked by a user of the vma
> > before it was recycled.
> > This write-locking should not cause performance issues because contention
> > during vma_mark_attached() can happen only in the rare vma reuse case.
> > Even when this happens, it's the slowpath (write-lock) which will be
> > waiting, not the page fault path.
> > After these provisions, SLAB_TYPESAFE_BY_RCU is added to vm_area_cachep.
> > This will facilitate vm_area_struct reuse and will minimize the number
> > of call_rcu() calls.
> > Adding a freeptr_t into vm_area_struct (unioned with vm_start/vm_end)
> > could be used to avoids bloating the structure, however currently
> > custom free pointers are not supported in combination with a ctor
> > (see the comment for kmem_cache_args.freeptr_offset).
> >
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
>
> I've stayed out of this discussion as my slab knowledge is far less than
> others (e.g. Vlastimil), but I shazam'd this series today and the kernel
> isn't booting on my qemu setup, and I bisected it to this commit, and an
> addr2line decode tells me this is:
>
> static inline void vma_mark_attached(struct vm_area_struct *vma)
> {
>         /* vma shoudn't be already attached */
>         VM_BUG_ON_VMA(!vma->detached, vma); <-------- here
>
>         ...
> }
>
> And if I go back to prior commits, this triggers too, since "mm: mark vma
> as detached until it's added into vma tree".
>
> I don't think this check should be added in this commit at any rate, it
> should be added in "mm: mark vma as detached until it's added into vma
> tree".

Agree, that patch is a better place to add this check and probably
locking as well. I think I know why I didn't catch this assertion in
my testing and how to fix it.

>
> I will go to that commit to continue this investigation, but we should
> definitely move that check over there.

Will reply to your comments in that commit.

>
> > ---
> >  include/linux/mm.h               | 48 ++++++++++++++++++++++++-----
> >  include/linux/mm_types.h         | 13 +++-----
> >  kernel/fork.c                    | 53 +++++++++++++++++++-------------
> >  mm/memory.c                      |  7 +++--
> >  mm/vma.c                         |  2 +-
> >  tools/testing/vma/vma_internal.h |  7 +++--
> >  6 files changed, 86 insertions(+), 44 deletions(-)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index dd1b6190df28..d8e10e1e34ad 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -257,7 +257,7 @@ struct vm_area_struct *vm_area_alloc(struct mm_struct *);
> >  struct vm_area_struct *vm_area_dup(struct vm_area_struct *);
> >  void vm_area_free(struct vm_area_struct *);
> >  /* Use only if VMA has no other users */
> > -void __vm_area_free(struct vm_area_struct *vma);
> > +void vm_area_free_unreachable(struct vm_area_struct *vma);
> >
> >  #ifndef CONFIG_MMU
> >  extern struct rb_root nommu_region_tree;
> > @@ -690,12 +690,32 @@ static inline void vma_lock_init(struct vm_area_struct *vma)
> >       vma->vm_lock_seq = UINT_MAX;
> >  }
> >
> > +#define VMA_BEFORE_LOCK              offsetof(struct vm_area_struct, vm_lock)
> > +#define VMA_LOCK_END(vma)    \
> > +     (((void *)(vma)) + offsetofend(struct vm_area_struct, vm_lock))
> > +#define VMA_AFTER_LOCK               \
> > +     (sizeof(struct vm_area_struct) - offsetofend(struct vm_area_struct, vm_lock))
> > +
> > +static inline void vma_clear(struct vm_area_struct *vma)
> > +{
> > +     /* Preserve vma->vm_lock */
> > +     memset(vma, 0, VMA_BEFORE_LOCK);
> > +     memset(VMA_LOCK_END(vma), 0, VMA_AFTER_LOCK);
> > +}
> > +
> > +static inline void vma_copy(struct vm_area_struct *new, struct vm_area_struct *orig)
> > +{
> > +     /* Preserve vma->vm_lock */
> > +     data_race(memcpy(new, orig, VMA_BEFORE_LOCK));
> > +     data_race(memcpy(VMA_LOCK_END(new), VMA_LOCK_END(orig), VMA_AFTER_LOCK));
> > +}
> > +
> >  /*
> >   * Try to read-lock a vma. The function is allowed to occasionally yield false
> >   * locked result to avoid performance overhead, in which case we fall back to
> >   * using mmap_lock. The function should never yield false unlocked result.
> >   */
> > -static inline bool vma_start_read(struct vm_area_struct *vma)
> > +static inline bool vma_start_read(struct mm_struct *mm, struct vm_area_struct *vma)
> >  {
> >       /*
> >        * Check before locking. A race might cause false locked result.
> > @@ -704,7 +724,7 @@ static inline bool vma_start_read(struct vm_area_struct *vma)
> >        * we don't rely on for anything - the mm_lock_seq read against which we
> >        * need ordering is below.
> >        */
> > -     if (READ_ONCE(vma->vm_lock_seq) == READ_ONCE(vma->vm_mm->mm_lock_seq.sequence))
> > +     if (READ_ONCE(vma->vm_lock_seq) == READ_ONCE(mm->mm_lock_seq.sequence))
> >               return false;
> >
> >       if (unlikely(down_read_trylock(&vma->vm_lock.lock) == 0))
> > @@ -721,7 +741,7 @@ static inline bool vma_start_read(struct vm_area_struct *vma)
> >        * after it has been unlocked.
> >        * This pairs with RELEASE semantics in vma_end_write_all().
> >        */
> > -     if (unlikely(vma->vm_lock_seq == raw_read_seqcount(&vma->vm_mm->mm_lock_seq))) {
> > +     if (unlikely(vma->vm_lock_seq == raw_read_seqcount(&mm->mm_lock_seq))) {
> >               up_read(&vma->vm_lock.lock);
> >               return false;
> >       }
> > @@ -810,7 +830,18 @@ static inline void vma_assert_locked(struct vm_area_struct *vma)
> >
> >  static inline void vma_mark_attached(struct vm_area_struct *vma)
> >  {
> > +     /* vma shoudn't be already attached */
> > +     VM_BUG_ON_VMA(!vma->detached, vma);
> > +
> > +     /*
> > +      * Lock here can be contended only if the vma got reused after
> > +      * lock_vma_under_rcu() found it but before it had a chance to
> > +      * read-lock it. Write-locking the vma guarantees that the vma
> > +      * won't be attached until all its old users are out.
> > +      */
> > +     down_write(&vma->vm_lock.lock);
> >       vma->detached = false;
> > +     up_write(&vma->vm_lock.lock);
> >  }
> >
> >  static inline void vma_mark_detached(struct vm_area_struct *vma)
> > @@ -847,7 +878,11 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
> >  #else /* CONFIG_PER_VMA_LOCK */
> >
> >  static inline void vma_lock_init(struct vm_area_struct *vma) {}
> > -static inline bool vma_start_read(struct vm_area_struct *vma)
> > +static inline void vma_clear(struct vm_area_struct *vma)
> > +             { memset(vma, 0, sizeof(*vma)); }
> > +static inline void vma_copy(struct vm_area_struct *new, struct vm_area_struct *orig)
> > +             { data_race(memcpy(new, orig, sizeof(*new))); }
> > +static inline bool vma_start_read(struct mm_struct *mm, struct vm_area_struct *vma)
> >               { return false; }
> >  static inline void vma_end_read(struct vm_area_struct *vma) {}
> >  static inline void vma_start_write(struct vm_area_struct *vma) {}
> > @@ -883,7 +918,7 @@ extern const struct vm_operations_struct vma_dummy_vm_ops;
> >
> >  static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
> >  {
> > -     memset(vma, 0, sizeof(*vma));
> > +     vma_clear(vma);
> >       vma->vm_mm = mm;
> >       vma->vm_ops = &vma_dummy_vm_ops;
> >       INIT_LIST_HEAD(&vma->anon_vma_chain);
> > @@ -892,7 +927,6 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
> >       vma->detached = true;
> >  #endif
> >       vma_numab_state_init(vma);
> > -     vma_lock_init(vma);
> >  }
> >
> >  /* Use when VMA is not part of the VMA tree and needs no locking */
> > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> > index 5c4bfdcfac72..8f6b0c935c2b 100644
> > --- a/include/linux/mm_types.h
> > +++ b/include/linux/mm_types.h
> > @@ -667,15 +667,10 @@ struct vma_numab_state {
> >  struct vm_area_struct {
> >       /* The first cache line has the info for VMA tree walking. */
> >
> > -     union {
> > -             struct {
> > -                     /* VMA covers [vm_start; vm_end) addresses within mm */
> > -                     unsigned long vm_start;
> > -                     unsigned long vm_end;
> > -             };
> > -#ifdef CONFIG_PER_VMA_LOCK
> > -             struct rcu_head vm_rcu; /* Used for deferred freeing. */
> > -#endif
> > +     struct {
> > +             /* VMA covers [vm_start; vm_end) addresses within mm */
> > +             unsigned long vm_start;
> > +             unsigned long vm_end;
> >       };
> >
> >       /*
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index f0cec673583c..76c68b041f8a 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -436,6 +436,11 @@ static struct kmem_cache *vm_area_cachep;
> >  /* SLAB cache for mm_struct structures (tsk->mm) */
> >  static struct kmem_cache *mm_cachep;
> >
> > +static void vm_area_ctor(void *data)
> > +{
> > +     vma_lock_init(data);
> > +}
> > +
> >  struct vm_area_struct *vm_area_alloc(struct mm_struct *mm)
> >  {
> >       struct vm_area_struct *vma;
> > @@ -462,8 +467,7 @@ struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig)
> >        * orig->shared.rb may be modified concurrently, but the clone
> >        * will be reinitialized.
> >        */
> > -     data_race(memcpy(new, orig, sizeof(*new)));
> > -     vma_lock_init(new);
> > +     vma_copy(new, orig);
> >       INIT_LIST_HEAD(&new->anon_vma_chain);
> >  #ifdef CONFIG_PER_VMA_LOCK
> >       /* vma is not locked, can't use vma_mark_detached() */
> > @@ -475,32 +479,37 @@ struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig)
> >       return new;
> >  }
> >
> > -void __vm_area_free(struct vm_area_struct *vma)
> > +static void __vm_area_free(struct vm_area_struct *vma, bool unreachable)
> >  {
> > +#ifdef CONFIG_PER_VMA_LOCK
> > +     /*
> > +      * With SLAB_TYPESAFE_BY_RCU, vma can be reused and we need
> > +      * vma->detached to be set before vma is returned into the cache.
> > +      * This way reused object won't be used by readers until it's
> > +      * initialized and reattached.
> > +      * If vma is unreachable, there can be no other users and we
> > +      * can set vma->detached directly with no risk of a race.
> > +      * If vma is reachable, then it should have been already detached
> > +      * under vma write-lock or it was never attached.
> > +      */
> > +     if (unreachable)
> > +             vma->detached = true;
> > +     else
> > +             VM_BUG_ON_VMA(!is_vma_detached(vma), vma);
> > +#endif
> >       vma_numab_state_free(vma);
> >       free_anon_vma_name(vma);
> >       kmem_cache_free(vm_area_cachep, vma);
> >  }
> >
> > -#ifdef CONFIG_PER_VMA_LOCK
> > -static void vm_area_free_rcu_cb(struct rcu_head *head)
> > +void vm_area_free(struct vm_area_struct *vma)
> >  {
> > -     struct vm_area_struct *vma = container_of(head, struct vm_area_struct,
> > -                                               vm_rcu);
> > -
> > -     /* The vma should not be locked while being destroyed. */
> > -     VM_BUG_ON_VMA(rwsem_is_locked(&vma->vm_lock.lock), vma);
> > -     __vm_area_free(vma);
> > +     __vm_area_free(vma, false);
> >  }
> > -#endif
> >
> > -void vm_area_free(struct vm_area_struct *vma)
> > +void vm_area_free_unreachable(struct vm_area_struct *vma)
> >  {
> > -#ifdef CONFIG_PER_VMA_LOCK
> > -     call_rcu(&vma->vm_rcu, vm_area_free_rcu_cb);
> > -#else
> > -     __vm_area_free(vma);
> > -#endif
> > +     __vm_area_free(vma, true);
> >  }
> >
> >  static void account_kernel_stack(struct task_struct *tsk, int account)
> > @@ -3135,9 +3144,11 @@ void __init proc_caches_init(void)
> >                       sizeof(struct fs_struct), 0,
> >                       SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_ACCOUNT,
> >                       NULL);
> > -     vm_area_cachep = KMEM_CACHE(vm_area_struct,
> > -                     SLAB_HWCACHE_ALIGN|SLAB_NO_MERGE|SLAB_PANIC|
> > -                     SLAB_ACCOUNT);
> > +     vm_area_cachep = kmem_cache_create("vm_area_struct",
> > +                     sizeof(struct vm_area_struct), 0,
> > +                     SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_TYPESAFE_BY_RCU|
> > +                     SLAB_ACCOUNT, vm_area_ctor);
> > +
> >       mmap_init();
> >       nsproxy_cache_init();
> >  }
> > diff --git a/mm/memory.c b/mm/memory.c
> > index d0197a0c0996..c8a3e820ed66 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -6275,7 +6275,7 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
> >       if (!vma)
> >               goto inval;
> >
> > -     if (!vma_start_read(vma))
> > +     if (!vma_start_read(mm, vma))
> >               goto inval;
> >
> >       /* Check if the VMA got isolated after we found it */
> > @@ -6292,8 +6292,9 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
> >        * fields are accessible for RCU readers.
> >        */
> >
> > -     /* Check since vm_start/vm_end might change before we lock the VMA */
> > -     if (unlikely(address < vma->vm_start || address >= vma->vm_end))
> > +     /* Check since vm_mm/vm_start/vm_end might change before we lock the VMA */
> > +     if (unlikely(vma->vm_mm != mm ||
> > +                  address < vma->vm_start || address >= vma->vm_end))
> >               goto inval_end_read;
> >
> >       rcu_read_unlock();
> > diff --git a/mm/vma.c b/mm/vma.c
> > index 73104d434567..050b83df3df2 100644
> > --- a/mm/vma.c
> > +++ b/mm/vma.c
> > @@ -382,7 +382,7 @@ void remove_vma(struct vm_area_struct *vma, bool unreachable)
> >               fput(vma->vm_file);
> >       mpol_put(vma_policy(vma));
> >       if (unreachable)
> > -             __vm_area_free(vma);
> > +             vm_area_free_unreachable(vma);
> >       else
> >               vm_area_free(vma);
> >  }
> > diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
> > index 2fed366d20ef..fd668d6cafc0 100644
> > --- a/tools/testing/vma/vma_internal.h
> > +++ b/tools/testing/vma/vma_internal.h
> > @@ -632,14 +632,15 @@ static inline void mpol_put(struct mempolicy *)
> >  {
> >  }
> >
> > -static inline void __vm_area_free(struct vm_area_struct *vma)
> > +static inline void vm_area_free(struct vm_area_struct *vma)
> >  {
> >       free(vma);
> >  }
> >
> > -static inline void vm_area_free(struct vm_area_struct *vma)
> > +static inline void vm_area_free_unreachable(struct vm_area_struct *vma)
> >  {
> > -     __vm_area_free(vma);
> > +     vma->detached = true;
> > +     vm_area_free(vma);
> >  }
> >
> >  static inline void lru_add_drain(void)
> > --
> > 2.47.0.338.g60cca15819-goog
> >

[PATCH v3 5/5] docs/mm: document latest changes to vm_lock

[Suren Baghdasaryan]

Change the documentation to reflect that vm_lock is integrated into vma.
Document newly introduced vma_start_read_locked{_nested} functions.

Signed-off-by: Suren Baghdasaryan <surenb@google.com>
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 Documentation/mm/process_addrs.rst | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/Documentation/mm/process_addrs.rst b/Documentation/mm/process_addrs.rst
index 1bf7ad010fc0..a18450b6496d 100644
--- a/Documentation/mm/process_addrs.rst
+++ b/Documentation/mm/process_addrs.rst
@@ -686,7 +686,11 @@ calls :c:func:`!rcu_read_lock` to ensure that the VMA is looked up in an RCU
 critical section, then attempts to VMA lock it via :c:func:`!vma_start_read`,
 before releasing the RCU lock via :c:func:`!rcu_read_unlock`.
 
-VMA read locks hold the read lock on the :c:member:`!vma->vm_lock` semaphore for
+In cases when the user already holds mmap read lock, :c:func:`!vma_start_read_locked`
+and :c:func:`!vma_start_read_locked_nested` can be used. These functions always
+succeed in acquiring VMA read lock.
+
+VMA read locks hold the read lock on the :c:member:`!vma.vm_lock` semaphore for
 their duration and the caller of :c:func:`!lock_vma_under_rcu` must release it
 via :c:func:`!vma_end_read`.
 
@@ -750,7 +754,7 @@ keep VMAs locked across entirely separate write operations. It also maintains
 correct lock ordering.
 
 Each time a VMA read lock is acquired, we acquire a read lock on the
-:c:member:`!vma->vm_lock` read/write semaphore and hold it, while checking that
+:c:member:`!vma.vm_lock` read/write semaphore and hold it, while checking that
 the sequence count of the VMA does not match that of the mm.
 
 If it does, the read lock fails. If it does not, we hold the lock, excluding
@@ -760,7 +764,7 @@ Importantly, maple tree operations performed in :c:func:`!lock_vma_under_rcu`
 are also RCU safe, so the whole read lock operation is guaranteed to function
 correctly.
 
-On the write side, we acquire a write lock on the :c:member:`!vma->vm_lock`
+On the write side, we acquire a write lock on the :c:member:`!vma.vm_lock`
 read/write semaphore, before setting the VMA's sequence number under this lock,
 also simultaneously holding the mmap write lock.
 
-- 
2.47.0.338.g60cca15819-goog