Message-ID: <6d010369-c0aa-458a-8b08-c14febea2f5f@lucifer.local>
Date: Mon, 18 Nov 2024 14:05:48 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Suren Baghdasaryan <surenb@...gle.com>
Cc: akpm@...ux-foundation.org, willy@...radead.org, liam.howlett@...cle.com,
        mhocko@...e.com, vbabka@...e.cz, hannes@...xchg.org, mjguzik@...il.com,
        oliver.sang@...el.com, mgorman@...hsingularity.net, david@...hat.com,
        peterx@...hat.com, oleg@...hat.com, dave@...olabs.net,
        paulmck@...nel.org, brauner@...nel.org, dhowells@...hat.com,
        hdanton@...a.com, hughd@...gle.com, minchan@...gle.com,
        jannh@...gle.com, shakeel.butt@...ux.dev, souravpanda@...gle.com,
        pasha.tatashin@...een.com, corbet@....net, linux-doc@...r.kernel.org,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        kernel-team@...roid.com
Subject: Re: [PATCH v3 4/5] mm: make vma cache SLAB_TYPESAFE_BY_RCU

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@...gle.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
>

Powered by blists - more mailing lists