Re: [PATCH 1/1] mm/ksm: validate KSM rmap items before hwpoison kill
From: Lorenzo Stoakes (ARM)
Date: Wed Aug 05 2026 - 12:34:48 EST
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 <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);
> > > }
> >
>
--
Cheers, Lorenzo