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

Hi Lorenzo,

Sorry, you are right. I messed up the process here -- I should not have
resent it so quickly, or sent v2 as a reply to v1.

I did use AI tooling while working on this, but the patch is my
responsibility. I did just follow the anonymous-page handling here, without
thinking it carefully enough.

Thanks,
Longlong


在 2026/8/6 0:47, Lorenzo Stoakes (ARM) 写道:
> :))) I did just say don't send a v2 but I guess you didn't see it.
>
> Basics:
>
> - Please don't respin when somebody's literally asked for feedback from somebody
>    else.
>
> - Please don't send v2 of a patch in-reply-to a v1 send it separately.
>
> - Please don't send a v2 on the same day as a v1.
>
> - Attach a changelog under the --- with a link to prior versions (using b4 makes
>    this easy).
>
> Anyway I guess I am forced to reply here... *grumble*.
>
> Thanks, Lorenzo
>
> On Thu, Aug 06, 2026 at 12:29:37AM +0800, 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.
> I'm very confused as to where memory poisoning comes into it? What exactly
> made you aware of this? Do you have a bug report you've observed?
>
>> Factor page_mapped_in_vma_at_address() out of page_mapped_in_vma() so
>> callers that already know the virtual address can validate it directly.
>> This avoids deriving the address from page_pgoff(), which is invalid
>> for KSM pages. Use the address saved in the KSM rmap item to check that
>> it still belongs to the VMA and that page_vma_mapped_walk() still finds
>> the poisoned page before adding the task to the kill list.
>>
>> Fixes: 4248d0083ec5 ("mm: ksm: support hwpoison for ksm page")
>> Suggested-by: David Hildenbrand (Arm) <[email protected]>
>> Signed-off-by: Longlong Xia <[email protected]>
> Honestly I have to ask - is this your own work or AI-generated? As I'm not
> confident you really understand this and it's tricky stuff so if a
> backportable patch is in the works I'd prefer somebody who understands it
> contributes it.
>
>> ---
>>   mm/internal.h        |  2 ++
>>   mm/ksm.c             | 10 +++++++---
>>   mm/page_vma_mapped.c | 36 +++++++++++++++++++++++-------------
>>   3 files changed, 32 insertions(+), 16 deletions(-)
>>
>> diff --git a/mm/internal.h b/mm/internal.h
>> index 181e79f1d6a2..4c9e601b2d95 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -1424,6 +1424,8 @@ void add_to_kill_ksm(struct task_struct *tsk, const struct page *p,
>>   		     unsigned long ksm_addr);
>>   unsigned long page_mapped_in_vma(const struct page *page,
>>   		struct vm_area_struct *vma);
>> +unsigned long page_mapped_in_vma_at_address(const struct page *page,
>> +		struct vm_area_struct *vma, unsigned long addr);
>>
>>   #else
>>   static inline int unmap_poisoned_folio(struct folio *folio, unsigned long pfn, bool must_kill)
>> diff --git a/mm/ksm.c b/mm/ksm.c
>> index 7d5b76478f0b..5104e442fcb2 100644
>> --- a/mm/ksm.c
>> +++ b/mm/ksm.c
>> @@ -3237,13 +3237,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) {
>> +		const unsigned long addr = rmap_item->address & PAGE_MASK;
>>   		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,9 +3253,13 @@ void collect_procs_ksm(const struct folio *folio, const struct page *page,
>>   			{
> OK so you're literally doing an anon rmap walk here, with the anon lock held.
>
>>   				vma = vmac->vma;
>>   				if (vma->vm_mm == t->mm) {
>> -					addr = rmap_item->address & PAGE_MASK;
>> +					const unsigned long mapped_addr =
>> +						page_mapped_in_vma_at_address(page, vma, addr);
> Now you're doing another anon rmap walk? Why on earth are you doing that? And
> won't this deadlock?
>
> Why aren't you just checking the whether addr is contained in the range here?
>
> Like:
>
> 	/* Make sure VMA wasn't split/remapped */
> 	if (!in_range(addr, vma->vm_start, vma_pages(vma)))
> 		continue;
>
> Or something?
>
>> +
>> +					if (mapped_addr == -EFAULT)
>> +						continue;
>>   					add_to_kill_ksm(t, page, vma, to_kill,
>> -							addr);
>> +							mapped_addr);
>>   				}
>>   			}
>>   		}
>> diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
>> index bac2eb5de63d..f7c5dc9248bc 100644
>> --- a/mm/page_vma_mapped.c
>> +++ b/mm/page_vma_mapped.c
>> @@ -336,6 +336,26 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)
>>   }
>>
>>   #ifdef CONFIG_MEMORY_FAILURE
>> +unsigned long page_mapped_in_vma_at_address(const struct page *page,
>> +		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 -EFAULT;
>> +	if (!page_vma_mapped_walk(&pvmw))
>> +		return -EFAULT;
>> +	page_vma_mapped_walk_done(&pvmw);
>> +
>> +	return pvmw.address;
>> +}
> I hate this name I hate that it's CONFIG_MEMORY_FAILURE only.
>
> Also it sounds like a predicate but returns an address? I have no idea what this
> is supposed to do? And no kdoc?...
>
>> +
>>   /**
>>    * page_mapped_in_vma - check whether a page is really mapped in a VMA
>>    * @page: the page to test
>> @@ -350,20 +370,10 @@ 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);
> Oh yes, make the !CONFIG_MEMORY_FAILURE build break *eye roll*
>
>>   }
>>   #endif
>> --
>> 2.43.0
>>
> --
> Cheers, Lorenzo