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

[Longlong Xia]

Hi Lorenzo,

Sorry, you are right. I messed up the process here -- I should not have
resent it so quickly, or sent v2 as a reply to v1.

I did use AI tooling while working on this, but the patch is my
responsibility. I did just follow the anonymous-page handling here, without
thinking it carefully enough.

Thanks,
Longlong


在 2026/8/6 0:47, Lorenzo Stoakes (ARM) 写道:
> :))) 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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Longlong Xia <xialonglong@xxxxxxxxxx>
> 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