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

[Longlong Xia]

  Hi David,

  Thanks for the review and for suggesting this approach.

  I will send v2 shortly with your Suggested-by tag.

  Thanks,
  Longlong

在 2026/8/5 20:19, David Hildenbrand (Arm) 写道:
> On 8/3/26 17:11, 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.
> >
> > 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 <xialonglong@xxxxxxxxxx>
> > ---
> >   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);
> >   				}