Re: [PATCH v2] mm/ksm: validate KSM rmap items before hwpoison kill

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

:))) I did just say don't send a v2 but I guess you didn't see it.

Basics:

- Please don't respin when somebody's literally asked for feedback from somebody
  else.

- Please don't send v2 of a patch in-reply-to a v1 send it separately.

- Please don't send a v2 on the same day as a v1.

- Attach a changelog under the --- with a link to prior versions (using b4 makes
  this easy).

Anyway I guess I am forced to reply here... *grumble*.

Thanks, Lorenzo

On Thu, Aug 06, 2026 at 12:29:37AM +0800, Longlong Xia wrote:
> From: Longlong Xia <xialonglong@kylinos.cn>
>
> collect_procs_ksm() walks the stable-node rmap list and queues an
> early kill for every task whose mm appears on the anon_vma chain.
>
> That rmap item can be stale by the time memory failure handles the
> poisoned KSM page. A VMA may have been split, unmapped or remapped
> after the rmap item was recorded, so matching only vma->vm_mm can send
> SIGBUS with an address that no longer maps the poisoned page.

I'm very confused as to where memory poisoning comes into it? What exactly
made you aware of this? Do you have a bug report you've observed?

>
> Factor page_mapped_in_vma_at_address() out of page_mapped_in_vma() so
> callers that already know the virtual address can validate it directly.
> This avoids deriving the address from page_pgoff(), which is invalid
> for KSM pages. Use the address saved in the KSM rmap item to check that
> it still belongs to the VMA and that page_vma_mapped_walk() still finds
> the poisoned page before adding the task to the kill list.
>
> Fixes: 4248d0083ec5 ("mm: ksm: support hwpoison for ksm page")
> Suggested-by: David Hildenbrand (Arm) <david@kernel.org>
> Signed-off-by: Longlong Xia <xialonglong@kylinos.cn>

Honestly I have to ask - is this your own work or AI-generated? As I'm not
confident you really understand this and it's tricky stuff so if a
backportable patch is in the works I'd prefer somebody who understands it
contributes it.

> ---
>  mm/internal.h        |  2 ++
>  mm/ksm.c             | 10 +++++++---
>  mm/page_vma_mapped.c | 36 +++++++++++++++++++++++-------------
>  3 files changed, 32 insertions(+), 16 deletions(-)
>
> diff --git a/mm/internal.h b/mm/internal.h
> index 181e79f1d6a2..4c9e601b2d95 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -1424,6 +1424,8 @@ void add_to_kill_ksm(struct task_struct *tsk, const struct page *p,
>  		     unsigned long ksm_addr);
>  unsigned long page_mapped_in_vma(const struct page *page,
>  		struct vm_area_struct *vma);
> +unsigned long page_mapped_in_vma_at_address(const struct page *page,
> +		struct vm_area_struct *vma, unsigned long addr);
>
>  #else
>  static inline int unmap_poisoned_folio(struct folio *folio, unsigned long pfn, bool must_kill)
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 7d5b76478f0b..5104e442fcb2 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -3237,13 +3237,13 @@ void collect_procs_ksm(const struct folio *folio, const struct page *page,
>  	if (!stable_node)
>  		return;
>  	hlist_for_each_entry(rmap_item, &stable_node->hlist, hlist) {
> +		const unsigned long addr = rmap_item->address & PAGE_MASK;
>  		struct anon_vma *av = rmap_item->anon_vma;
>
>  		anon_vma_lock_read(av);
>  		rcu_read_lock();
>  		for_each_process(tsk) {
>  			struct anon_vma_chain *vmac;
> -			unsigned long addr;
>  			struct task_struct *t =
>  				task_early_kill(tsk, force_early);
>  			if (!t)
> @@ -3253,9 +3253,13 @@ void collect_procs_ksm(const struct folio *folio, const struct page *page,
>  			{

OK so you're literally doing an anon rmap walk here, with the anon lock held.

>  				vma = vmac->vma;
>  				if (vma->vm_mm == t->mm) {
> -					addr = rmap_item->address & PAGE_MASK;
> +					const unsigned long mapped_addr =
> +						page_mapped_in_vma_at_address(page, vma, addr);

Now you're doing another anon rmap walk? Why on earth are you doing that? And
won't this deadlock?

Why aren't you just checking the whether addr is contained in the range here?

Like:

	/* Make sure VMA wasn't split/remapped */
	if (!in_range(addr, vma->vm_start, vma_pages(vma)))
		continue;

Or something?

> +
> +					if (mapped_addr == -EFAULT)
> +						continue;
>  					add_to_kill_ksm(t, page, vma, to_kill,
> -							addr);
> +							mapped_addr);
>  				}
>  			}
>  		}
> diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
> index bac2eb5de63d..f7c5dc9248bc 100644
> --- a/mm/page_vma_mapped.c
> +++ b/mm/page_vma_mapped.c
> @@ -336,6 +336,26 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)
>  }
>
>  #ifdef CONFIG_MEMORY_FAILURE
> +unsigned long page_mapped_in_vma_at_address(const struct page *page,
> +		struct vm_area_struct *vma, unsigned long addr)
> +{
> +	struct page_vma_mapped_walk pvmw = {
> +		.pfn = page_to_pfn(page),
> +		.nr_pages = 1,
> +		.vma = vma,
> +		.address = addr,
> +		.flags = PVMW_SYNC,
> +	};
> +
> +	if (addr < vma->vm_start || addr >= vma->vm_end)
> +		return -EFAULT;
> +	if (!page_vma_mapped_walk(&pvmw))
> +		return -EFAULT;
> +	page_vma_mapped_walk_done(&pvmw);
> +
> +	return pvmw.address;
> +}

I hate this name I hate that it's CONFIG_MEMORY_FAILURE only.

Also it sounds like a predicate but returns an address? I have no idea what this
is supposed to do? And no kdoc?...

> +
>  /**
>   * page_mapped_in_vma - check whether a page is really mapped in a VMA
>   * @page: the page to test
> @@ -350,20 +370,10 @@ unsigned long page_mapped_in_vma(const struct page *page,
>  		struct vm_area_struct *vma)
>  {
>  	const struct folio *folio = page_folio(page);
> -	struct page_vma_mapped_walk pvmw = {
> -		.pfn = page_to_pfn(page),
> -		.nr_pages = 1,
> -		.vma = vma,
> -		.flags = PVMW_SYNC,
> -	};
> +	const unsigned long addr = vma_address(vma, page_pgoff(folio, page), 1);
>
> -	pvmw.address = vma_address(vma, page_pgoff(folio, page), 1);
> -	if (pvmw.address == -EFAULT)
> -		goto out;
> -	if (!page_vma_mapped_walk(&pvmw))
> +	if (addr == -EFAULT)
>  		return -EFAULT;
> -	page_vma_mapped_walk_done(&pvmw);
> -out:
> -	return pvmw.address;
> +	return page_mapped_in_vma_at_address(page, vma, addr);

Oh yes, make the !CONFIG_MEMORY_FAILURE build break *eye roll*

>  }
>  #endif
> --
> 2.43.0
>

--
Cheers, Lorenzo