Re: [PATCH 3/4] mm/char-mem: implement huge_fault support for /dev/mem

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mm/char-mem: read-only PTE after fixup_user_fault() in
  mmap_mem_access()
--

commit 2398e62524e61afe6489b128b93a5b0922afcd83
Author: Eliav Farber <farbere@amazon.com>

mm/char-mem: implement huge_fault support for /dev/mem

This commit switches non-COW /dev/mem mappings from eager remap at mmap()
time to fault-time population using ->fault and ->huge_fault handlers, and
adds mmap_mem_access() to fault in untouched pages before calling
generic_access_phys().

> diff --git a/mm/char-mem.c b/mm/char-mem.c
> index c5761ddedbd7d..b9ed8473baeb7 100644
> --- a/mm/char-mem.c
> +++ b/mm/char-mem.c
> @@ -318,9 +318,92 @@ static inline int private_mapping_ok(struct vm_area_desc *desc)

[ ... ]

> +#ifdef CONFIG_HAVE_IOREMAP_PROT
> +/*
> + * generic_access_phys() only follows present PTEs, and a lazily populated
> + * mapping has none for pages the target has not touched yet. Fault the page in
> + * first, so that remote access (ptrace, /proc/pid/mem) keeps working.
> + *
> + * fixup_user_fault() refuses a fault the vma does not permit, so an untouched
> + * page of a mapping without VM_READ stays unreadable, where the eager mapping
> + * left a present PTE.
> + */
> +static int mmap_mem_access(struct vm_area_struct *vma, unsigned long addr,
> +			   void *buf, int len, int write)
> +{
> +	unsigned int fault_flags = FAULT_FLAG_REMOTE;
> +
> +	if (write)
> +		fault_flags |= FAULT_FLAG_WRITE;
> +
> +	/*
> +	 * COW mappings are populated at mmap() time, and a write fault would
> +	 * copy the page instead. With unlocked == NULL, fixup_user_fault()
> +	 * keeps the mmap lock held and the vma valid.
> +	 */
> +	if (!vma_is_cow_mapping(vma))
> +		fixup_user_fault(vma->vm_mm, addr, fault_flags, NULL);
> +
> +	return generic_access_phys(vma, addr, buf, len, write);
> +}
> +#endif

[Severity: Medium]
Can a remote write to an untouched order-0 page fail with -EINVAL when
soft-dirty tracking is armed on the VMA?

Writing 4 to /proc/pid/clear_refs clears VM_SOFTDIRTY on the VMA and strips
write permission from vma->vm_page_prot via vma_set_page_prot().  On an
order-0 fault, vmf_insert_pfn_order() calls vmf_insert_pfn(), which calls
insert_pfn() with mkwrite == false and installs a read-only PTE:

mm/memory.c:vmf_insert_pfn_order() {
	struct vm_area_struct *vma = vmf->vma;
	bool write = vmf->flags & FAULT_FLAG_WRITE;

	if (!order)
		return vmf_insert_pfn(vma, vmf->address, pfn);

fixup_user_fault() only invokes handle_mm_fault() once, so it does not take
the second write-protection fault (do_wp_page() -> wp_pfn_shared()) that a
CPU store would take to make the PTE writable.  generic_access_phys() then
sees !args.writable with FOLL_WRITE set and returns -EINVAL on the first
write attempt.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111113045.35368-1-farbere@amazon.com?part=3