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

[Longlong Xia <xialonglong2025@163.com>]

   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@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);
>>   				}
>