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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

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?

>  			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).

> +			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.

>
>  	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_.

>
>  	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.

Maybe this code needs some refactoring though in general... yikes.

> +
>  	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.

>  		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?

>  		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_ :)

There are callers that don't care at all if the merge failed, whether through
oom or VMA write lock not being acquired.

There's really no benefit in exiting early here I don't think, the subsequent
split will call vma_start_write_killable() anyway.

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?

> +	} 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
>