On Mon, Nov 18, 2024 at 6:06 AM Lorenzo Stoakes
<lorenzo.stoakes@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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
> >
