On Tue, Nov 19, 2024 at 04:08:24PM -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@xxxxxxxxxx>
I tested this (whole series) locally and on real hardware and did a kernel
compile on real hardware just to be sure :)) and all looks good.
The code looks sensible, so:
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> ---
> 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;
> +}
Yeah I think sensible to just accept that sometimes we are already attached
when we mark attached.
> +
> 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
>
