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

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

>> @@ -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?

I think it's the same as with page_mapped_in_vma(): it expects the anon vma lock
to already haven been taken.

At least that's what I understand when looking at collect_procs_anon. :)

> 
> Why aren't you just checking the whether addr is contained in the range here?

Yeah, that should be much easier.

> 
> 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.

Note that page_mapped_in_vma() is also KSM only.

But I am curious why the function differs from what I quickly hacked together.


-- 
Cheers,

David