[Devel] [PATCH hci-8.1] mm: allow MADV_DOEXEC on shared PFN mappings of device memory

[Devel] [PATCH hci-8.1] mm: allow MADV_DOEXEC on shared PFN mappings of device memory

[Andrey Drobyshev]

QEMU live update (cpr-exec) keeps guest RAM across exec with MADV_DOEXEC,
but VFIO device BARs mapped into QEMU are lost on exec, and the new QEMU
has to mmap them again on the same device fd.

Some drivers don't allow that.  The NVIDIA vGPU VFIO driver records each
BAR mapping and refuses an overlapping one with -EINVAL.  For VF BARs the
record is dropped only when the device fd is closed, so after exec the
new QEMU can't map BAR1/BAR3 and the vGPU becomes unusable.

Let MADV_DOEXEC also preserve shared VM_PFNMAP mappings of a file, such
as VFIO BARs.  They have no struct pages and nothing to copy-on-write, so
vma_dup() duplicates them the same way fork does for a shared file
mapping: take a file reference, link the VMA into the file's i_mmap (so
unmap_mapping_range() on device reset still reaches it), call
vm_ops->open() and copy the PTEs pointing at the device.

Such a VMA may have no vm_ops (when filled with remap_pfn_range() at mmap
time), in which case vma_is_anonymous() returns true for it.  So the new
vma_is_shared_pfnmap() helper is checked first.

https://virtuozzo.atlassian.net/browse/VSTOR-103860
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Feature: oracle/mm: MADV_DOEXEC madvise() flag
---
 include/linux/mm.h | 12 ++++++++++++
 mm/madvise.c       | 11 +++++++----
 mm/mmap.c          | 38 +++++++++++++++++++++++++++++---------
 3 files changed, 48 insertions(+), 13 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 75b8bcd9f442..0f3ad979f980 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1018,6 +1018,18 @@ static inline bool vma_is_anonymous(struct vm_area_struct *vma)
 	return !vma->vm_ops;
 }
 
+/*
+ * Shared mapping of memory without struct pages, such as a device BAR
+ * mapped through VFIO.  Such a VMA may still have no vm_ops (when the
+ * driver populates it with remap_pfn_range() at mmap time), so it must
+ * be tested before vma_is_anonymous().
+ */
+static inline bool vma_is_shared_pfnmap(struct vm_area_struct *vma)
+{
+	return vma->vm_file &&
+	       (vma->vm_flags & (VM_SHARED | VM_PFNMAP)) == (VM_SHARED | VM_PFNMAP);
+}
+
 /*
  * Indicate if the VMA is a heap for the given task; for
  * /proc/PID/maps that is the heap of the main task.
diff --git a/mm/madvise.c b/mm/madvise.c
index b89d891e122c..f24004e3c6f2 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1309,11 +1309,14 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
 		break;
 	case MADV_DOEXEC:
 		/*
-		 * MADV_DOEXEC is only supported on private, non-executable,
-		 * non-stack anonymous memory and if the VM_EXEC_KEEP flag
-		 * is available.
+		 * MADV_DOEXEC is supported on private, non-executable,
+		 * non-stack anonymous memory, and on shared PFN mappings
+		 * of device memory (e.g. VFIO BARs), if the VM_EXEC_KEEP
+		 * flag is available.
 		 */
-		if (!VM_EXEC_KEEP || !vma_is_anonymous(vma) || vma->vm_flags & (VM_EXEC|VM_SHARED|VM_STACK)) {
+		if (!VM_EXEC_KEEP || vma->vm_flags & (VM_EXEC | VM_STACK) ||
+		    !(vma_is_shared_pfnmap(vma) ||
+		      (vma_is_anonymous(vma) && !(vma->vm_flags & VM_SHARED)))) {
 			error = -EINVAL;
 			goto out;
 		}
diff --git a/mm/mmap.c b/mm/mmap.c
index 320f397816df..fd73b80b04e1 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1751,16 +1751,21 @@ int insert_vm_struct(struct mm_struct *mm, struct vm_area_struct *vma)
 }
 
 /*
- * Duplicate an anonymous VMA into a new mm as part of preserving
- * it across exec.
+ * Duplicate a VMA into a new mm as part of preserving it across exec.
+ *
+ * Anonymous VMAs get their pages shared with the new mm fork-style.
+ * Shared PFN mappings of device memory (e.g. VFIO BARs) are duplicated
+ * like fork duplicates a shared file mapping: same file, same PTEs
+ * pointing at the device, nothing to copy-on-write.
  */
 int vma_dup(struct vm_area_struct *old_vma, struct mm_struct *mm)
 {
 	struct vm_area_struct *vma;
 	unsigned long npages;
+	bool pfnmap = vma_is_shared_pfnmap(old_vma);
 	int ret = -ENOMEM;
 
-	if (WARN_ON(!vma_is_anonymous(old_vma)))
+	if (WARN_ON(!pfnmap && !vma_is_anonymous(old_vma)))
 		return -EINVAL;
 
 	if (find_vma_intersection(mm, old_vma->vm_start, old_vma->vm_end))
@@ -1778,9 +1783,13 @@ int vma_dup(struct vm_area_struct *old_vma, struct mm_struct *mm)
 		goto fail_nomem_policy;
 
 	vma->vm_mm = mm;
-	ret = anon_vma_fork(vma, old_vma);
-	if (ret)
-		goto fail_nomem_anon_vma_fork;
+	if (pfnmap) {
+		get_file(vma->vm_file);
+	} else {
+		ret = anon_vma_fork(vma, old_vma);
+		if (ret)
+			goto fail_nomem_anon_vma_fork;
+	}
 
 	/*
 	 * Clear functionality that should not carry over to the new
@@ -1791,10 +1800,14 @@ int vma_dup(struct vm_area_struct *old_vma, struct mm_struct *mm)
 	vm_flags_clear(vma, VM_LOCKED|VM_LOCKONFAULT|VM_UFFD_MISSING|VM_UFFD_WP|VM_UFFD_MINOR);
 	vma->vm_userfaultfd_ctx = NULL_VM_UFFD_CTX;
 
+	/* Also links a file VMA into the file's i_mmap, for unmap_mapping_range() */
 	ret = vma_link(mm, vma);
 	if (ret)
 		goto fail_nomem_vma_link;
 
+	if (pfnmap && vma->vm_ops && vma->vm_ops->open)
+		vma->vm_ops->open(vma);
+
 	/*
 	 * Now that the dup vma is inserted into the mm, clear VM_ACCOUNT
 	 * from old_vma.  Since vma_dup() is only called during exec to
@@ -1807,17 +1820,24 @@ int vma_dup(struct vm_area_struct *old_vma, struct mm_struct *mm)
 	 */
 	__vm_flags_mod(old_vma, 0, VM_ACCOUNT);
 
+	/* For PFN maps this copies the device PTEs and the PAT memtype */
+	if (pfnmap)
+		ret = copy_page_range(vma, old_vma);
+	else
 #ifdef CONFIG_PADATA
-	ret = copy_page_range_mt(vma, old_vma);
+		ret = copy_page_range_mt(vma, old_vma);
 #else
-	ret = copy_page_range(vma, old_vma);
+		ret = copy_page_range(vma, old_vma);
 #endif
 	vm_flags_clear(vma, VM_EXEC_KEEP);
 
 	return ret;
 
 fail_nomem_vma_link:
-	unlink_anon_vmas(vma);
+	if (pfnmap)
+		fput(vma->vm_file);
+	else
+		unlink_anon_vmas(vma);
 fail_nomem_anon_vma_fork:
 	mpol_put(vma_policy(vma));
 fail_nomem_policy:
-- 
2.47.1

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1] mm: allow MADV_DOEXEC on shared PFN mappings of device memory

[no-reply]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mm: MADV_DOEXEC dups any shared PFNMAP VMA, sharing vm_private_data across mms
- [Medium] mm: vma_dup() links pfnmap VMA into i_mmap before PTE copy, so zap can miss them
- [Low] mm: vma_dup() keeps inherited VM_PAT on PFNMAP copy, unlike dup_mmap()
--

commit 2e5fc626303801168dc124d9f0c079510fb5d4fd
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

mm: allow MADV_DOEXEC on shared PFN mappings of device memory

This patch lets MADV_DOEXEC also preserve shared VM_PFNMAP file mappings
such as VFIO BARs. vma_dup() duplicates them the way fork does for a shared
file mapping: it takes a file reference, links the VMA into i_mmap, calls
vm_ops->open() and copies the PTEs.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-103860

> diff --git a/include/linux/mm.h b/include/linux/mm.h
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1018,6 +1018,18 @@ static inline bool vma_is_anonymous(struct vm_area_struct *vma)
>  	return !vma->vm_ops;
>  }
>  
> +/*
> + * Shared mapping of memory without struct pages, such as a device BAR
> + * mapped through VFIO.  Such a VMA may still have no vm_ops (when the
> + * driver populates it with remap_pfn_range() at mmap time), so it must
> + * be tested before vma_is_anonymous().
> + */
> +static inline bool vma_is_shared_pfnmap(struct vm_area_struct *vma)
> +{
> +	return vma->vm_file &&
> +	       (vma->vm_flags & (VM_SHARED | VM_PFNMAP)) == (VM_SHARED | VM_PFNMAP);
> +}
> +

[Severity: High]
The eligibility test here and in madvise_vma_behavior() is purely
structural: a file plus VM_SHARED|VM_PFNMAP. There is no driver opt-in, and
VM_DONTCOPY and VM_IO are not excluded. Can this let unprivileged userspace
opt any driver PFN mapping into exec-time duplication?

vma_dup() copies the VMA with vm_area_dup(), so vm_private_data is shared
verbatim between the old and new VMA. Only vm_ops->open() is called, never
->mmap(). Drivers that free per-mapping state in ->close() without a
refcounting ->open() then get two VMAs and two ->close() calls.

For example, gru_file_mmap() sets VM_IO|VM_PFNMAP|VM_DONTCOPY and stores
vdata in vm_private_data. gru_vma_close() does:

    vdata = vma->vm_private_data;
    vma->vm_private_data = NULL;
    ...
    kfree(vdata);

There is no ->open(). The duplicated VMA keeps the stale vdata pointer, so
can its later close double free vdata?

genwqe_mmap() uses remap_pfn_range(), and genwqe_vma_open() is an empty
stub. genwqe_vma_close() runs at exec teardown of the old mm:

    __genwqe_del_mapping(cfile, dma_map);
    __genwqe_free_consistent(cd, dma_map->size, dma_map->k_vaddr,
                             dma_map->dma_addr);
    kfree(dma_map);

Can this free the coherent DMA buffer while the duplicated PFNMAP VMA in the
new mm still maps it, leaving userspace with access to freed memory?

uacce sets VM_DONTCOPY|VM_WIPEONFORK and kfree()s its qfr on close. Its
->mmap() uniqueness check is bypassed by the duplication, so can the
hardware mapping outlive its qfr?

Drivers set VM_DONTCOPY to say the VMA should not be duplicated.
MADV_DOFORK rejects VM_IO. Should this path require an explicit driver
opt-in, or at least reject VM_DONTCOPY and VM_IO VMAs, or require ->open()
to be defined?

> diff --git a/mm/mmap.c b/mm/mmap.c
> --- a/mm/mmap.c
> +++ b/mm/mmap.c

[ ... ]

>  int vma_dup(struct vm_area_struct *old_vma, struct mm_struct *mm)
>  {
>  	struct vm_area_struct *vma;
>  	unsigned long npages;
> +	bool pfnmap = vma_is_shared_pfnmap(old_vma);
>  	int ret = -ENOMEM;

[ ... ]

>  	vma->vm_mm = mm;
> -	ret = anon_vma_fork(vma, old_vma);
> -	if (ret)
> -		goto fail_nomem_anon_vma_fork;
> +	if (pfnmap) {
> +		get_file(vma->vm_file);
> +	} else {

[Severity: Low]
In dup_mmap(), a VM_PFNMAP VMA gets this before copy_page_range():

    /* track_pfn_copy() will later take care of copying internal state. */
    if (unlikely(tmp->vm_flags & VM_PFNMAP))
            untrack_pfn_clear(tmp);

The pfnmap branch here has no equivalent, so the vm_area_dup() copy keeps
the inherited VM_PAT flag without owning a reservation. Can this leave an
inconsistent PAT state on the destination VMA?

Consider copy_page_range() failing in track_pfn_copy() (in get_pat_info()
or reserve_pfn_range()) before it reaches vm_flags_set(dst_vma, VM_PAT).
The exec then fails and the new mm is torn down, and unmap_single_vma()
calls untrack_pfn() on a VMA that still has the inherited VM_PAT.
get_pat_info() runs on empty destination page tables. Can this hit
WARN_ON_ONCE(1) on the exec error path, and trigger a panic with
panic_on_warn? Should vma_dup() call untrack_pfn_clear() on the new VMA
before copy_page_range() as dup_mmap() does?

[ ... ]

> +	/* Also links a file VMA into the file's i_mmap, for unmap_mapping_range() */
>  	ret = vma_link(mm, vma);
>  	if (ret)
>  		goto fail_nomem_vma_link;
>  
> +	if (pfnmap && vma->vm_ops && vma->vm_ops->open)
> +		vma->vm_ops->open(vma);
> +

[ ... ]

> +	/* For PFN maps this copies the device PTEs and the PAT memtype */
> +	if (pfnmap)
> +		ret = copy_page_range(vma, old_vma);
> +	else

[Severity: Medium]
vma_link() makes the new VMA visible in i_mmap via vma_link_file(), which
takes i_mmap_rwsem for write only around the insert. copy_page_range() then
runs with no i_mmap_rwsem held. Can unmap_mapping_range() miss the copied
PTEs?

unmap_mapping_pages() takes only i_mmap_rwsem for read and no mmap_lock.
One possible interleaving:

    exec thread                        unmap_mapping_range()
    vma_link()  (new VMA inserted)
                                       walks i_mmap, zaps empty new VMA
    copy_page_range()  (copies PTEs
      from old_vma, not yet zapped)
                                       zaps old_vma only

The new mm would then keep live device MMIO PTEs after the revoke. fork()
avoids this with:

    vma_interval_tree_insert_after(tmp, mpnt, &mapping->i_mmap);

so the walker sees the parent before the child. vma_link_file() uses a
plain insert with no such ordering. Is there something that closes this
window, such as keeping i_mmap_rwsem held across the PTE copy or ordering
the new VMA after old_vma?

>  #ifdef CONFIG_PADATA
> -	ret = copy_page_range_mt(vma, old_vma);
> +		ret = copy_page_range_mt(vma, old_vma);
>  #else
> -	ret = copy_page_range(vma, old_vma);
> +		ret = copy_page_range(vma, old_vma);
>  #endif

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009143227.492150-1-andrey.drobyshev@virtuozzo.com?part=1

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel