[PATCH RFC v4 07/12] mm: read remote strings under the per-VMA lock

[Rik van Riel]

__copy_remote_vm_str() reads another process's memory under the mmap read
lock for the whole copy, and looks the VMA up again for every page through
get_user_page_lookup_vma().

This is the mmap-lock-only form that __access_remote_vm() used before it
moved to the per-VMA lock, and it shares the same contention: an mmap() or
munmap() stalls a bpf_copy_from_user_task_str() reader even though the
string is resident.

Read the string through remote_vm_walk(), the walk __access_remote_vm()
already uses, with a copy_vm_str() action that strscpy()s each page and
stops at the NUL. Bounding the copy by its maximum length keeps the per-VMA
path inside a single VMA, so no VMA is looked up again while that lock is
held.

A copy the per-VMA lock cannot finish falls back to the mmap lock: a fault
that dropped the lock, or a string running past the VMA. Memory with no
struct page (VM_IO/VM_PFNMAP) and stack expansion stay on the mmap lock
path and still return -EFAULT, as before; ->access() support is left for
later.

Assisted-by: Claude:claude-opus-4.8
Signed-off-by: Rik van Riel <riel@xxxxxxxxxxx>
---
 mm/memory.c | 111 ++++++++++++++++++++++------------------------------
 1 file changed, 47 insertions(+), 64 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 273dfe12bc6d..aaf620017f34 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -7296,84 +7296,67 @@ EXPORT_SYMBOL_GPL(access_process_vm);
 
 #ifdef CONFIG_BPF_SYSCALL
 /*
- * Copy a string from another process's address space as given in mm.
- * If there is any error return -EFAULT.
+ * Copy a NUL-terminated string from @addr into *@buf, up to @len bytes,
+ * stopping at the NUL. strscpy() always NUL terminates, so recopy the last
+ * byte of a page when more pages follow. A string is never read from
+ * struct-page-less VM_IO / VM_PFNMAP memory.
  */
-static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
-				void *buf, int len, unsigned int gup_flags)
+static int copy_vm_str(struct vm_area_struct *vma, struct page *page,
+		       unsigned long addr, void **buf, int len, int write)
 {
-	void *old_buf = buf;
-	int err = 0;
-
-	*(char *)buf = '\0';
+	struct folio *folio;
+	int bytes, offset, retval;
+	void *maddr;
 
-	if (mmap_read_lock_killable(mm))
+	if (!page)
 		return -EFAULT;
 
-	addr = untagged_addr_remote(mm, addr);
+	bytes = len;
+	offset = addr & (PAGE_SIZE - 1);
+	if (bytes > PAGE_SIZE - offset)
+		bytes = PAGE_SIZE - offset;
 
-	/* Avoid triggering the temporary warning in __get_user_pages */
-	if (!vma_lookup(mm, addr)) {
-		err = -EFAULT;
-		goto out;
+	folio = page_folio(page);
+	maddr = kmap_local_folio(folio, folio_page_idx(folio, page) * PAGE_SIZE);
+	retval = strscpy(*buf, maddr + offset, bytes);
+	if (retval >= 0) {
+		/* Found the end of the string. */
+		*buf += retval;
+		folio_release_kmap(folio, maddr);
+		return 0;
 	}
 
-	while (len) {
-		int bytes, offset, retval;
-		void *maddr;
-		struct folio *folio;
-		struct page *page;
-		struct vm_area_struct *vma = NULL;
+	*buf += bytes - 1;
+	if (bytes != len) {
+		copy_from_user_page(vma, page, addr + bytes - 1, *buf,
+				    maddr + (PAGE_SIZE - 1), 1);
+		*buf += 1;
+	}
+	folio_release_kmap(folio, maddr);
 
-		page = get_user_page_lookup_vma(mm, addr, gup_flags, &vma);
-		if (IS_ERR(page)) {
-			/*
-			 * Treat as a total failure for now until we decide how
-			 * to handle the CONFIG_HAVE_IOREMAP_PROT case and
-			 * stack expansion.
-			 */
-			*(char *)buf = '\0';
-			err = -EFAULT;
-			goto out;
-		}
+	return bytes;
+}
 
-		folio = page_folio(page);
-		bytes = len;
-		offset = addr & (PAGE_SIZE - 1);
-		if (bytes > PAGE_SIZE - offset)
-			bytes = PAGE_SIZE - offset;
-
-		maddr = kmap_local_folio(folio, folio_page_idx(folio, page) * PAGE_SIZE);
-		retval = strscpy(buf, maddr + offset, bytes);
-		if (retval >= 0) {
-			/* Found the end of the string */
-			buf += retval;
-			folio_release_kmap(folio, maddr);
-			break;
-		}
+/*
+ * Copy a string from another process's address space as given in mm.
+ * If there is any error return -EFAULT.
+ */
+static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
+				void *buf, int len, unsigned int gup_flags)
+{
+	int bytes, err;
 
-		buf += bytes - 1;
-		/*
-		 * Because strscpy always NUL terminates we need to
-		 * copy the last byte in the page if we are going to
-		 * load more pages
-		 */
-		if (bytes != len) {
-			addr += bytes - 1;
-			copy_from_user_page(vma, page, addr, buf, maddr + (PAGE_SIZE - 1), 1);
-			buf += 1;
-			addr += 1;
-		}
-		len -= bytes;
+	*(char *)buf = '\0';
 
-		folio_release_kmap(folio, maddr);
+	bytes = remote_vm_walk(mm, addr, buf, len, gup_flags, false,
+			       copy_vm_str, &err);
+	if (err) {
+		/* The contract guarantees a terminated buffer even on error. */
+		((char *)buf)[bytes] = '\0';
+		return err;
 	}
 
-out:
-	mmap_read_unlock(mm);
-	if (err)
-		return err;
-	return buf - old_buf;
+	return bytes;
 }
 
 /**
-- 
2.53.0-Meta