Re: [PATCH] mm: don't ioremap COWed anon pages in generic_access_phys()

[David Hildenbrand (Arm)]

On 10/2/26 10:38, Lorenzo Stoakes (ARM) wrote:
> On Thu, Oct 01, 2026 at 10:22:29PM +0200, David Hildenbrand (Arm) wrote:
>> On 10/1/26 17:25, Nguyen Ngoc Thang wrote:
>>> A MAP_PRIVATE mapping of iomem (e.g. a PCI sysfs resourceN file) is a
>>> COW pfnmap: a write fault replaces the pfn with an anonymous page, which
>>> remap_pfn_range() allows by keeping vm_pgoff equal to the base pfn.
>>>
>>> generic_access_phys() does not tell those COWed pages apart from the
>>> original pfns and ioremaps whatever the PTE points to. Reading such an
>>> address via /proc/pid/mem or ptrace then ioremaps RAM:
>>>
>>>   ioremap on RAM at 0x0000000045623000 - 0x0000000045623fff
>>>   WARNING: arch/x86/mm/ioremap.c:216 at __ioremap_caller.isra.0+0x4c2/0x5f0
>>>   Call Trace:
>>>    generic_access_phys+0x130/0x4d0 mm/memory.c:7178
>>>    kernfs_vma_access+0x1ce/0x280 fs/kernfs/file.c:437
>>>    __access_remote_vm+0x58f/0x890 mm/memory.c:7256
>>>    mem_rw+0x2a1/0x670 fs/proc/base.c:912
>>>
>>
> 
> Sorry to say this looks schlopped.
> 
> This guy has sent 10 series across 6 subsystems over ~21 hrs:
> 
> https://lore.kernel.org/all/?q=f%3Angocthang2710.1999%40gmail.com
> 
> Nguyen - please do not flood the kernel with patches, and please use the
> Assisted-by tag for generated content.
> 
> The original code you submitted is really not great even if the issue may
> be valid.
> 
> So I'd say somebody from the core team should take over this if we want to
> come up with a patch.

Yes, I'll take care of it.

[...]
>>
>> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> 
> This looks reasonable but I hate that we have 'special' CoW overrides like
> this :)

After sending this yesterday, I concluded that we can do this cleaner: just have

bool normal_page;

(naming suggestions?)

that express that this is something refcounted with a struct page, like
documented for vm_normal_page().

Then we can just refuse all of these.

> 
>> ---
>>  include/linux/mm.h |  4 +++-
>>  mm/memory.c        | 48 ++++++++++++++++++++++++++++++++++++++++------
>>  2 files changed, 45 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index c49ef99b4413..b90e547797a9 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -3193,6 +3193,8 @@ struct folio *vm_normal_folio_pmd(struct vm_area_struct *vma,
>>  				  unsigned long addr, pmd_t pmd);
>>  struct page *vm_normal_page_pmd(struct vm_area_struct *vma, unsigned long addr,
>>  				pmd_t pmd);
>> +struct folio *vm_normal_folio_pud(struct vm_area_struct *vma,
>> +		unsigned long addr, pud_t pud);
> 
> Hmm, if not defined before why would this need a new PUD handler? Do we
> even have PUD-leaf PFN mappings?

Yes we do. In any case, good for consistency.

[...]

>> +	args->cow = folio && folio_test_anon(folio);
> 
> I actually wonder if this should be changed to folio_has_anon_rmap() at
> some point :)

Not a fan.

> 
> Since a folio being 'anon' is vague, because we stupidly made 'anon' vague
> in general.

For folios it's an established term :)

> 
> Anyway I was going to ask does this suffice for CoW but having an anon rmap
> implies CoW so it does.


The downside of using "bool normal_page;" is that we should check
vm_normal_page() for any mapping, not just cow mappings. I suspect
performance-wise we don't really care.

-- 
Cheers,

David