Re: [PATCH v3 3/3] mm: use vma_start_write_killable() in process_vma_walk_lock()

["Lorenzo Stoakes (Oracle)" <ljs@kernel.org>]

On Tue, Mar 03, 2026 at 03:59:17PM -0800, Suren Baghdasaryan wrote:
> On Mon, Mar 2, 2026 at 7:25 AM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > On Wed, Feb 25, 2026 at 11:06:09PM -0800, Suren Baghdasaryan wrote:
> > > Replace vma_start_write() with vma_start_write_killable() when
> > > process_vma_walk_lock() is used with PGWALK_WRLOCK option.
> > > Adjust its direct and indirect users to check for a possible error
> > > and handle it. Ensure users handle EINTR correctly and do not ignore
> > > it.
> > >
> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> >
> > Have raised concerns below but also this feels like you're trying to do a bit
> > too much in one patch here, probably worth splitting out based on the different
> > parts you changed.
> >
> > > ---
> > >  arch/s390/kvm/kvm-s390.c |  2 +-
> > >  fs/proc/task_mmu.c       |  5 ++++-
> > >  mm/mempolicy.c           | 14 +++++++++++---
> > >  mm/pagewalk.c            | 20 ++++++++++++++------
> > >  mm/vma.c                 | 22 ++++++++++++++--------
> > >  mm/vma.h                 |  6 ++++++
> > >  6 files changed, 50 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > > index 7a175d86cef0..337e4f7db63a 100644
> > > --- a/arch/s390/kvm/kvm-s390.c
> > > +++ b/arch/s390/kvm/kvm-s390.c
> > > @@ -2948,7 +2948,7 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> > >               }
> > >               /* must be called without kvm->lock */
> > >               r = kvm_s390_handle_pv(kvm, &args);
> > > -             if (copy_to_user(argp, &args, sizeof(args))) {
> > > +             if (r != -EINTR && copy_to_user(argp, &args, sizeof(args))) {
> >
> > This is horribly ugly, and if we were already filtering any other instance of
> > -EINTR (if they're even possible from copy_to_user()) why is -EINTR being
> > treated in a special way?
> >
> > I honestly _hate_ this if (errcode != -EINTR) { ... } pattern in general, I'd
> > really rather we didn't.
> >
> > It's going to bitrot and people are going to assume it's for some _very good
> > reason_ and nobody will understand why it's getting special treatment...
> >
> > Surely a fatal signal would have previously resulted in -EFAULT before which is
> > a similar situation so most consistent would be to keep filtering no?
>
> Current code ignores any error coming from kvm_s390_handle_pv() and
> proceeds with copy_to_user(), possibly overriding the former error. I
> don't really know if this is an oversight or an intentional behavior,
> so I wanted to minimize possible side effects. I guess I should try to
> fix it properly (or learn why this was done this way). I'll post a
> separate patch to error out immediately if kvm_s390_handle_pv() fails
> and will ask s390 experts for review.

Thanks!

>
> >
> > >                       r = -EFAULT;
> > >                       break;
> > >               }
> > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > > index e091931d7ca1..1238a2988eb6 100644
> > > --- a/fs/proc/task_mmu.c
> > > +++ b/fs/proc/task_mmu.c
> > > @@ -1797,6 +1797,7 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
> > >               struct clear_refs_private cp = {
> > >                       .type = type,
> > >               };
> > > +             int err;
> > >
> > >               if (mmap_write_lock_killable(mm)) {
> > >                       count = -EINTR;
> > > @@ -1824,7 +1825,9 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
> > >                                               0, mm, 0, -1UL);
> > >                       mmu_notifier_invalidate_range_start(&range);
> > >               }
> > > -             walk_page_range(mm, 0, -1, &clear_refs_walk_ops, &cp);
> > > +             err = walk_page_range(mm, 0, -1, &clear_refs_walk_ops, &cp);
> > > +             if (err < 0)
> >
> > Again with this < 0 :) let's be consistent, if (err).
>
> Ack.

Thanks!

>
> >
> > > +                     count = err;
> > >               if (type == CLEAR_REFS_SOFT_DIRTY) {
> > >                       mmu_notifier_invalidate_range_end(&range);
> > >                       flush_tlb_mm(mm);
> > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> > > index 90939f5bde02..3c8b3dfc9c56 100644
> > > --- a/mm/mempolicy.c
> > > +++ b/mm/mempolicy.c
> > > @@ -988,6 +988,8 @@ queue_pages_range(struct mm_struct *mm, unsigned long start, unsigned long end,
> > >                       &queue_pages_lock_vma_walk_ops : &queue_pages_walk_ops;
> >
> > There's a comment above:
> >
> >  * queue_pages_range() may return:
> >  * 0 - all pages already on the right node, or successfully queued for moving
> >  *     (or neither strict checking nor moving requested: only range checking).
> >  * >0 - this number of misplaced folios could not be queued for moving
> >  *      (a hugetlbfs page or a transparent huge page being counted as 1).
> >  * -EIO - a misplaced page found, when MPOL_MF_STRICT specified without MOVEs.
> >  * -EFAULT - a hole in the memory range, when MPOL_MF_DISCONTIG_OK unspecified.
> >  */
> >
> > You should add the -EINTR to it.
>
> Ack.

Thanks!

>
> >
> > >
> > >       err = walk_page_range(mm, start, end, ops, &qp);
> > > +     if (err == -EINTR)
> > > +             return err;
> >
> > Again, you're special casing without really any justification here. Let's please
> > not special case -EINTR unless you have a _really good_ reason to.
> >
> > And also - if we fail to walk the page range because we couldn't get a VMA write
> > lock, that's ok. The walk failed. There's nothing to unlock, because we didn't
> > even get the write lock in the first place, so there's no broken state, it's as
> > if we failed at some other point right?
> >
> > So I don't see why we're special casing this _at all_.
>
> I want to avoid possible -EINTR code override with -EFAULT in the code below.
> walk_page_range() can return -EINVAL and any other error that
> ops->pte_hole or ops->test_walk might return. We might be fine
> treating all of them as -EFAULT but masking -EINTR seems wrong to me.
> I don't really know a better way to deal with this but if you have a
> good alternative I would really appreciate it.

As per Matthew we needn't worry, and in any case if we want to check for fatal
signal early exit we can do if (fatal_signal_pending(current)) {} I think?

>
> >
> > >
> > >       if (!qp.first)
> > >               /* whole range in hole */
> > > @@ -1309,9 +1311,14 @@ static long migrate_to_node(struct mm_struct *mm, int source, int dest,
> > >                                     flags | MPOL_MF_DISCONTIG_OK, &pagelist);
> > >       mmap_read_unlock(mm);
> >
> >
> > >
> > > +     if (nr_failed == -EINTR)
> > > +             err = nr_failed;
> >
> > Ugh please don't, that's REALLY horrible.
> >
> > Actually the only way you'd get a write lock happening in the walk_page_range()
> > is if flags & MPOL_MF_WRLOCK, menaing queue_pages_lock_vma_walk_ops are used
> > which specifies .walk_lock = PGWALK_WRLOCK.
> >
> > And this flag is only set in do_mbind(), not in migrate_to_node().
> >
> > So this check is actually totally unnecessary. You'll never get -EINTR here.
>
> Ah, good point. I'll drop this part.

Thanks!

>
> >
> > Maybe this code needs some refactoring though in general... yikes.
>
> Right.
>
> >
> > > +
> > >       if (!list_empty(&pagelist)) {
> > > -             err = migrate_pages(&pagelist, alloc_migration_target, NULL,
> > > -                     (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL, NULL);
> > > +             if (!err)
> > > +                     err = migrate_pages(&pagelist, alloc_migration_target,
> > > +                                         NULL, (unsigned long)&mtc,
> > > +                                         MIGRATE_SYNC, MR_SYSCALL, NULL);
> >
> > Given the above, this is unnecessary too.
>
> Ack. Will drop.

Thanks!

>
> >
> > >               if (err)
> > >                       putback_movable_pages(&pagelist);
> > >       }
> > > @@ -1611,7 +1618,8 @@ static long do_mbind(unsigned long start, unsigned long len,
> > >                               MR_MEMPOLICY_MBIND, NULL);
> > >       }
> > >
> > > -     if (nr_failed && (flags & MPOL_MF_STRICT))
> > > +     /* Do not mask EINTR */
> >
> > Useless comment... You're not explaining why, and it's obvious what you're doing.
> >
> > > +     if ((err != -EINTR) && (nr_failed && (flags & MPOL_MF_STRICT)))
> >
> > Weird use of parens...
> >
> > And again why are we treating -EINTR in a special way?
>
> Ah, actually I don't think I need this here. If queue_pages_range()
> fails nr_failed gets reset to 0, so the original error won't be masked
> as -EIO. I'll drop this part.

Thanks!

>
> >
> > >               err = -EIO;
> > >       if (!list_empty(&pagelist))
> > >               putback_movable_pages(&pagelist);
> > > diff --git a/mm/pagewalk.c b/mm/pagewalk.c
> > > index a94c401ab2cf..dc9f7a7709c6 100644
> > > --- a/mm/pagewalk.c
> > > +++ b/mm/pagewalk.c
> > > @@ -425,14 +425,13 @@ static inline void process_mm_walk_lock(struct mm_struct *mm,
> > >               mmap_assert_write_locked(mm);
> > >  }
> > >
> > > -static inline void process_vma_walk_lock(struct vm_area_struct *vma,
> > > +static inline int process_vma_walk_lock(struct vm_area_struct *vma,
> > >                                        enum page_walk_lock walk_lock)
> > >  {
> > >  #ifdef CONFIG_PER_VMA_LOCK
> > >       switch (walk_lock) {
> > >       case PGWALK_WRLOCK:
> > > -             vma_start_write(vma);
> > > -             break;
> > > +             return vma_start_write_killable(vma);
> > >       case PGWALK_WRLOCK_VERIFY:
> > >               vma_assert_write_locked(vma);
> > >               break;
> > > @@ -444,6 +443,7 @@ static inline void process_vma_walk_lock(struct vm_area_struct *vma,
> > >               break;
> > >       }
> > >  #endif
> > > +     return 0;
> > >  }
> > >
> > >  /*
> > > @@ -487,7 +487,9 @@ int walk_page_range_mm_unsafe(struct mm_struct *mm, unsigned long start,
> > >                       if (ops->pte_hole)
> > >                               err = ops->pte_hole(start, next, -1, &walk);
> > >               } else { /* inside vma */
> > > -                     process_vma_walk_lock(vma, ops->walk_lock);
> > > +                     err = process_vma_walk_lock(vma, ops->walk_lock);
> > > +                     if (err)
> > > +                             break;
> > >                       walk.vma = vma;
> > >                       next = min(end, vma->vm_end);
> > >                       vma = find_vma(mm, vma->vm_end);
> > > @@ -704,6 +706,7 @@ int walk_page_range_vma_unsafe(struct vm_area_struct *vma, unsigned long start,
> > >               .vma            = vma,
> > >               .private        = private,
> > >       };
> > > +     int err;
> > >
> > >       if (start >= end || !walk.mm)
> > >               return -EINVAL;
> > > @@ -711,7 +714,9 @@ int walk_page_range_vma_unsafe(struct vm_area_struct *vma, unsigned long start,
> > >               return -EINVAL;
> > >
> > >       process_mm_walk_lock(walk.mm, ops->walk_lock);
> > > -     process_vma_walk_lock(vma, ops->walk_lock);
> > > +     err = process_vma_walk_lock(vma, ops->walk_lock);
> > > +     if (err)
> > > +             return err;
> > >       return __walk_page_range(start, end, &walk);
> > >  }
> > >
> > > @@ -734,6 +739,7 @@ int walk_page_vma(struct vm_area_struct *vma, const struct mm_walk_ops *ops,
> > >               .vma            = vma,
> > >               .private        = private,
> > >       };
> > > +     int err;
> > >
> > >       if (!walk.mm)
> > >               return -EINVAL;
> > > @@ -741,7 +747,9 @@ int walk_page_vma(struct vm_area_struct *vma, const struct mm_walk_ops *ops,
> > >               return -EINVAL;
> > >
> > >       process_mm_walk_lock(walk.mm, ops->walk_lock);
> > > -     process_vma_walk_lock(vma, ops->walk_lock);
> > > +     err = process_vma_walk_lock(vma, ops->walk_lock);
> > > +     if (err)
> > > +             return err;
> > >       return __walk_page_range(vma->vm_start, vma->vm_end, &walk);
> > >  }
> > >
> > > diff --git a/mm/vma.c b/mm/vma.c
> > > index 9f2664f1d078..46bbad6e64a4 100644
> > > --- a/mm/vma.c
> > > +++ b/mm/vma.c
> > > @@ -998,14 +998,18 @@ static __must_check struct vm_area_struct *vma_merge_existing_range(
> > >       if (anon_dup)
> > >               unlink_anon_vmas(anon_dup);
> > >
> > > -     /*
> > > -      * This means we have failed to clone anon_vma's correctly, but no
> > > -      * actual changes to VMAs have occurred, so no harm no foul - if the
> > > -      * user doesn't want this reported and instead just wants to give up on
> > > -      * the merge, allow it.
> > > -      */
> > > -     if (!vmg->give_up_on_oom)
> > > -             vmg->state = VMA_MERGE_ERROR_NOMEM;
> > > +     if (err == -EINTR) {
> > > +             vmg->state = VMA_MERGE_ERROR_INTR;
> >
> > Yeah this is incorrect. You seem adament in passing through -EINTR _no
> > matter what_ :)
>
> You got me figured out ;)
>
> >
> > There are callers that don't care at all if the merge failed, whether through
> > oom or VMA write lock not being acquired.
>
> Ah, I see. I was a bit puzzled by this vmg->give_up_on_oom flag. I
> think what you are saying is that errors from
> vma_merge_existing_range() are ignored unless this flag is set and
> even then the only possible error is ENOMEM.
>
> >
> > There's really no benefit in exiting early here I don't think, the subsequent
> > split will call vma_start_write_killable() anyway.
>
> But are we always calling split after the merge?

We wouldn't if start == vma->vm_start and end == vma->vm_end but that'd be a nop
anyway :) [in vma_modify(), the only caller].

>
> >
> > So I think this adds a lot of complexity and mess for nothing.
> >
> > So can we drop all this change to the merge logic please?
>
> Ok but is there a good reason for this unusual error handling logic in
> vma_merge_existing_range()?

It's specifically so we can indicate _why_ the merge didn't succeed, because the
function returns NULL. Is checked in vma_modify().

Better this way than an ERR_PTR().


>
> >
> > > +     } else {
> > > +             /*
> > > +              * This means we have failed to clone anon_vma's correctly,
> > > +              * but no actual changes to VMAs have occurred, so no harm no
> > > +              * foul - if the user doesn't want this reported and instead
> > > +              * just wants to give up on the merge, allow it.
> > > +              */
> > > +             if (!vmg->give_up_on_oom)
> > > +                     vmg->state = VMA_MERGE_ERROR_NOMEM;
> > > +     }
> > >       return NULL;
> > >  }
> > >
> > > @@ -1681,6 +1685,8 @@ static struct vm_area_struct *vma_modify(struct vma_merge_struct *vmg)
> > >       merged = vma_merge_existing_range(vmg);
> > >       if (merged)
> > >               return merged;
> > > +     if (vmg_intr(vmg))
> > > +             return ERR_PTR(-EINTR);
> > >       if (vmg_nomem(vmg))
> > >               return ERR_PTR(-ENOMEM);
> > >
> > > diff --git a/mm/vma.h b/mm/vma.h
> > > index eba388c61ef4..fe4560f81f4f 100644
> > > --- a/mm/vma.h
> > > +++ b/mm/vma.h
> > > @@ -56,6 +56,7 @@ struct vma_munmap_struct {
> > >  enum vma_merge_state {
> > >       VMA_MERGE_START,
> > >       VMA_MERGE_ERROR_NOMEM,
> > > +     VMA_MERGE_ERROR_INTR,
> > >       VMA_MERGE_NOMERGE,
> > >       VMA_MERGE_SUCCESS,
> > >  };
> > > @@ -226,6 +227,11 @@ static inline bool vmg_nomem(struct vma_merge_struct *vmg)
> > >       return vmg->state == VMA_MERGE_ERROR_NOMEM;
> > >  }
> > >
> > > +static inline bool vmg_intr(struct vma_merge_struct *vmg)
> > > +{
> > > +     return vmg->state == VMA_MERGE_ERROR_INTR;
> > > +}
> > > +
> > >  /* Assumes addr >= vma->vm_start. */
> > >  static inline pgoff_t vma_pgoff_offset(struct vm_area_struct *vma,
> > >                                      unsigned long addr)
> > > --
> > > 2.53.0.414.gf7e9f6c205-goog
> > >
> >

Cheers, Lorenzo