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

["David Hildenbrand (Arm)" <david@kernel.org>]

On 8/3/26 17:11, 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.
> 
> 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@kylinos.cn>
> ---
>  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,

David