Re: [PATCH 3/3] mm: check for VMA being detached before destroying it

[Suren Baghdasaryan]

On Tue, Jun 20, 2023 at 4:57 PM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote:
>
> By the time VMA is freed it has to be detached with the exception of
> exit_mmap which is destroying the whole VMA tree. Enforce this
> requirement before freeing the VMA. exit_mmap in the only user calling
> __vm_area_free directly, therefore it won't trigger the new check.
> Change VMA initialization to mark new VMAs as detached and change that
> flag once the VMA is added into a tree.
>
> Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>

My tests did not generate the warning but the test coverage is far
from perfect, so if someone can run extensive testing on this one that
would be greatly appreciated.
Thanks,
Suren.

> ---
>  include/linux/mm.h | 4 ++--
>  kernel/fork.c      | 2 ++
>  mm/internal.h      | 1 +
>  3 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 74e3033c9fc2..9a10fcdb134e 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -247,7 +247,7 @@ void setup_initial_init_mm(void *start_code, void *end_code,
>  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 */
> +/* Use only if VMA has no other users and might still be attached to a tree */
>  void __vm_area_free(struct vm_area_struct *vma);
>
>  #ifndef CONFIG_MMU
> @@ -751,7 +751,7 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm)
>         vma->vm_mm = mm;
>         vma->vm_ops = &dummy_vm_ops;
>         INIT_LIST_HEAD(&vma->anon_vma_chain);
> -       vma_mark_detached(vma, false);
> +       vma->detached = true;
>         vma_numab_state_init(vma);
>  }
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 41c964104b58..000fc429345c 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -540,6 +540,7 @@ static void vm_area_free_rcu_cb(struct rcu_head *head)
>
>         /* The vma should not be locked while being destroyed. */
>         VM_BUG_ON_VMA(rwsem_is_locked(&vma->vm_lock->lock), vma);
> +       WARN_ON_ONCE(!vma->detached);
>         __vm_area_free(vma);
>  }
>  #endif
> @@ -549,6 +550,7 @@ void vm_area_free(struct vm_area_struct *vma)
>  #ifdef CONFIG_PER_VMA_LOCK
>         call_rcu(&vma->vm_rcu, vm_area_free_rcu_cb);
>  #else
> +       WARN_ON_ONCE(!vma->detached);
>         __vm_area_free(vma);
>  #endif
>  }
> diff --git a/mm/internal.h b/mm/internal.h
> index 68410c6d97ac..728189e6c703 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -1068,6 +1068,7 @@ static inline void vma_iter_store(struct vma_iterator *vmi,
>         vmi->mas.index = vma->vm_start;
>         vmi->mas.last = vma->vm_end - 1;
>         mas_store_prealloc(&vmi->mas, vma);
> +       vma_mark_detached(vma, false);
>  }
>
>  static inline int vma_iter_store_gfp(struct vma_iterator *vmi,
> --
> 2.41.0.162.gfafddb0af9-goog
>