On Fri, Nov 22, 2024 at 8:47 AM Lorenzo Stoakes
<lorenzo.stoakes@xxxxxxxxxx> wrote:
>
> 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>
Thanks Lorenzo! I'll post a new version today, addressing Matthew's
and other's comments but overall functionality should stay the same.
>
> > ---
> > 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
> >
