On Tue, Nov 12, 2024 at 11:46:33AM -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.
>
This seems very sensible.
> Signed-off-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
Aside from nits/refactoring suggestions below:
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> ---
> include/linux/mm.h | 10 +++++++++-
> mm/memory.c | 2 +-
> mm/mmap.c | 2 ++
> mm/nommu.c | 2 ++
> mm/vma.c | 3 +++
> tools/testing/vma/vma_internal.h | 3 ++-
Just want to say THANK YOU for taking the time to update the testing stubs :)
Much appreciated!
> 6 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index a5eb0be3e351..245a85caf4c3 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -812,6 +812,11 @@ static inline void vma_mark_detached(struct vm_area_struct *vma, bool detached)
> vma->detached = detached;
> }
>
> +static inline bool is_vma_detached(struct vm_area_struct *vma)
> +{
> + return vma->detached;
> +}
> +
> static inline void release_fault_lock(struct vm_fault *vmf)
> {
> if (vmf->flags & FAULT_FLAG_VMA_LOCK)
> @@ -874,7 +879,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);
How did this work before? Oh I guess we initialised the VMA lock earlier right?
> +#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);
> }
> 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/mmap.c b/mm/mmap.c
> index 386429f7db5a..1295c4cedaf4 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1570,6 +1570,7 @@ static int do_brk_flags(struct vma_iterator *vmi, struct vm_area_struct *vma,
> if (vma_iter_store_gfp(vmi, vma, GFP_KERNEL))
> goto mas_store_fail;
>
> + vma_mark_detached(vma, false);
> mm->map_count++;
> validate_mm(mm);
> ksm_add_vma(vma);
> @@ -1890,6 +1891,7 @@ static struct vm_area_struct *__install_special_mapping(
> if (ret)
> goto out;
>
> + vma_mark_detached(vma, false);
similar to vma_iter_store() comment, maybe worht putting in insert_vm_struct()?
> vm_stat_account(mm, vma->vm_flags, len >> PAGE_SHIFT);
>
> perf_event_mmap(vma);
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 9cb6e99215e2..6afd5c2bd97d 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -1192,6 +1192,7 @@ unsigned long do_mmap(struct file *file,
> current->mm->map_count++;
> /* add the VMA to the tree */
> vma_iter_store(&vmi, vma);
> + vma_mark_detached(vma, false);
Since we to seem always to do this with vma_iter_store() do we want to put this
there? Or maybe make a wrapper around the two if that seems not to separate
concerns enough?
>
> /* we flush the region from the icache only when the first executable
> * mapping of it is made */
> @@ -1357,6 +1358,7 @@ static int split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma,
> setup_vma_to_mm(vma, mm);
> setup_vma_to_mm(new, mm);
> vma_iter_store(vmi, new);
> + vma_mark_detached(new, false);
> mm->map_count++;
> return 0;
>
> diff --git a/mm/vma.c b/mm/vma.c
> index 8a454a7bbc80..1426871fa6e0 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -275,6 +275,7 @@ static void vma_complete(struct vma_prepare *vp, struct vma_iterator *vmi,
> * (it may either follow vma or precede it).
> */
> vma_iter_store(vmi, vp->insert);
> + vma_mark_detached(vp->insert, false);
> mm->map_count++;
> }
>
> @@ -1690,6 +1691,7 @@ int vma_link(struct mm_struct *mm, struct vm_area_struct *vma)
>
> vma_start_write(vma);
> vma_iter_store(&vmi, vma);
> + vma_mark_detached(vma, false);
> vma_link_file(vma);
> mm->map_count++;
> validate_mm(mm);
> @@ -2369,6 +2371,7 @@ static int __mmap_new_vma(struct mmap_state *map, struct vm_area_struct **vmap)
> /* Lock the VMA since it is modified after insertion into VMA tree */
> vma_start_write(vma);
> vma_iter_store(vmi, vma);
> + vma_mark_detached(vma, false);
> map->mm->map_count++;
> vma_link_file(vma);
>
> diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
> index 1d9fc97b8e80..fdb60978821f 100644
> --- a/tools/testing/vma/vma_internal.h
> +++ b/tools/testing/vma/vma_internal.h
> @@ -438,7 +438,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;
You're the best :)
> }
>
> static inline struct vm_area_struct *vm_area_alloc(struct mm_struct *mm)
> --
> 2.47.0.277.g8800431eea-goog
>
