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

On Thu, Aug 06, 2026 at 12:21:24AM +0800, Longlong Xia wrote:
>   Hi David,
>
>   Thanks for the review and for suggesting this approach.
>
>   I will send v2 shortly with your Suggested-by tag.

Hang on :) I wanted to look at this (always better to wait at least a day before
respins in general).

>
>   Thanks,
>   Longlong
>
> 在 2026/8/5 20:19, David Hildenbrand (Arm) 写道:
> > On 8/3/26 17:11, Longlong Xia wrote:
> > > From: Longlong Xia <[email protected]>
> > >
> > > 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.
> > >
> > > Check that the saved address still belongs to the VMA and that
> > > page_vma_mapped_walk() still finds the poisoned page there before
> > > adding the task to the kill list.
> > >
> > > Fixes: 4248d0083ec5 ("mm: ksm: support hwpoison for ksm page")
> > > Signed-off-by: Longlong Xia <[email protected]>
> > > ---
> > >   mm/ksm.c | 27 +++++++++++++++++++++++++--
> > >   1 file changed, 25 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/mm/ksm.c b/mm/ksm.c
> > > index 7d5b76478f0b..bc4b2dd894d8 100644
> > > --- a/mm/ksm.c
> > > +++ b/mm/ksm.c
> > > @@ -3222,6 +3222,27 @@ void rmap_walk_ksm(struct folio *folio, struct rmap_walk_control *rwc)
> > >   }
> > >   #ifdef CONFIG_MEMORY_FAILURE
> > > +static bool ksm_rmap_item_mapped(const struct page *page,
> > > +				 struct vm_area_struct *vma,
> > > +				 unsigned long addr)
> > Two tab indent on second parameter line
> >
> > 	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 false;
> > > +	if (!page_vma_mapped_walk(&pvmw))
> > > +		return false;
> > > +	page_vma_mapped_walk_done(&pvmw);
> > > +
> > We have page_mapped_in_vma(). So I wonder whether we can find a way to
> >
> > 1) Modify to just work with KSM (CCing Lorenzo)
> >
> > Maybe it already does. I'm confused as so often.
> >
> > Looking at the existing caller collect_procs_anon(), it's really only called
> > on anon folios. Could it already be called on KSM folios? What would happen
> > in that case? (does it just work because folio->index is still what we expect)
> >
> > 2) Do the following
> >
> > diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
> > index d7670ba4147bf..7eeb3c336cfe9 100644
> > --- a/mm/page_vma_mapped.c
> > +++ b/mm/page_vma_mapped.c
> > @@ -342,6 +342,27 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)
> >   }
> >   #ifdef CONFIG_MEMORY_FAILURE
> > +static unsigned long page_mapped_in_vma_at_address(const struct page *page,
> > +               struct vm_area_struct *vma, unsigned long addr)
> > +{
> > +       const struct folio *folio = page_folio(page);
> > +       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);
> > +out:
> > +       return pvmw.address;
> > +}
> > +
> >   /**
> >    * page_mapped_in_vma - check whether a page is really mapped in a VMA
> >    * @page: the page to test
> > @@ -355,21 +376,10 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)
> >   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);
> >   }
> >   #endif
> >
> >
> > > +	return true;
> > > +}
> > > +
> > >   /*
> > >    * Collect processes when the error hit an ksm page.
> > >    */
> > > @@ -3237,13 +3258,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) {
> > > +		unsigned long addr = rmap_item->address & PAGE_MASK;
> > Can be const.
> >
> > >   		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,7 +3274,9 @@ void collect_procs_ksm(const struct folio *folio, const struct page *page,
> > >   			{
> > >   				vma = vmac->vma;
> > >   				if (vma->vm_mm == t->mm) {
> > > -					addr = rmap_item->address & PAGE_MASK;
> > > +					if (!ksm_rmap_item_mapped(page, vma,
> > > +								  addr))
> > jut put that onto a single line, please: easier to read.
> >
> > > +						continue;
> > >   					add_to_kill_ksm(t, page, vma, to_kill,
> > >   							addr);
> > >   				}
> >
>

--
Cheers, Lorenzo