From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CB5B430DD1F for ; Tue, 2 Dec 2025 10:45:17 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764672317; cv=none; b=gWC8aM1f9KwAlc9WA9FLjVth5VK2w9/wN1Bij6/Zqx3VRuRjM6dKmaz5JL68nBoqksHDTiGh9ziONu90imV2Mhlx/km1mpW7Ev/7xYa4cK6vxXPdX6QN8c1x+3iUK/s5hYUXjzYG5psl43052HR/bAG5pRthq/coQiTgjGX11mg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764672317; c=relaxed/simple; bh=eFKHQt1gkMWz2B16+a2YkhZvuXqFwfrnAsK7xxmrnS0=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=h9VgDxiX7qZps6uQD/KBxN5GfRktdKwikMTt+kFU6cbQ+q19GdPcqgH+Ij9PKmkuaTTi9fW/QcMv+Xd9ALr9Ij0VWrHZLW9aNC5bdyQE++64iyYXYRH6Z7BloXq9KbEn0Q+UwudHXMZyNF7tNF6fLOtEGjvdkkIQ/9jyKnlKf2E= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=vJdo9i/J; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="vJdo9i/J" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 0D85BC4CEF1; Tue, 2 Dec 2025 10:45:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1764672317; bh=eFKHQt1gkMWz2B16+a2YkhZvuXqFwfrnAsK7xxmrnS0=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=vJdo9i/J8Bb0OIkOFAHO0UEmybPDdLWINtZZvLb4OKPhDHrwpkgVi4IEJBazA6G5B 9PjhkR9TjvjoyPcyEaVuhYTy4MO5t6cEpzquPltpQsYF9GzQiZgFZRNyUQKHImeDBY CdmbdygsNmGUeILfdp5hZ9Oa25WgjPnzEmYXvDyAWStXAwgwuNQqQru3frYn1wAXnr vTN0HfuujdVEgzC+0VgcjUSQ5EDCYXlT+D1+QUNCGu66JHi/xSN0xNtWMqwpFj/REO G+q1mgN8j1AExpcjVQMR4k1liAyHSQ1UMwJwrt01xJN6AUbMbRDIqVI5weKTAFm6tQ IIDvGhR2NIx6g== Message-ID: <9d963e84-9740-4bc6-9d9e-98ba4528cae4@kernel.org> Date: Tue, 2 Dec 2025 11:45:10 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] process_vm_readv/writev: add flags for pidfd and nowait To: Alban Crequy , Andrew Morton , Christian Brauner Cc: Lorenzo Stoakes , "Liam R . Howlett" , Vlastimil Babka , Mike Rapoport , Suren Baghdasaryan , Michal Hocko , linux-kernel@vger.kernel.org, linux-mm@kvack.org, Alban Crequy , Peter Xu , Willy Tarreau , mfriese@microsoft.com References: <20251118132348.2415603-1-alban.crequy@gmail.com> From: "David Hildenbrand (Red Hat)" Content-Language: en-US In-Reply-To: <20251118132348.2415603-1-alban.crequy@gmail.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 11/18/25 14:23, Alban Crequy wrote: > From: Alban Crequy > Hi, subject: would probably better be something like: "mm/process_vm_access: pidfd and nowait support for process_vm_readv/writev" > - PROCESS_VM_PIDFD: refer to the remote process via PID file descriptor > instead of PID. Such a file descriptor can be obtained with > pidfd_open(2). > > - PROCESS_VM_NOWAIT: do not block on IO if the memory access causes a > page fault. > > If a given flag is unsupported, the syscall returns the error EINVAL > without checking the buffers. This gives a way to userspace to detect > whether the current kernel supports a specific flag: > > process_vm_readv(pid, NULL, 1, NULL, 1, PROCESS_VM_PIDFD) > -> EINVAL if the kernel does not support the flag PROCESS_VM_PIDFD > (before this patch) > -> EFAULT if the kernel supports the flag (after this patch) > You should not just describe what you are doing, but also why you are doing it. What is the problem you are trying to solve in user space where you would require these extensions? > Signed-off-by: Alban Crequy > --- > MAINTAINERS | 1 + > include/uapi/linux/process_vm.h | 9 +++++++++ > mm/process_vm_access.c | 20 +++++++++++++++----- > 3 files changed, 25 insertions(+), 5 deletions(-) > create mode 100644 include/uapi/linux/process_vm.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index e64b94e6b5a9..91b4647cf761 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -16272,6 +16272,7 @@ F: include/linux/pgtable.h > F: include/linux/ptdump.h > F: include/linux/vmpressure.h > F: include/linux/vmstat.h > +F: include/uapi/linux/process_vm.h > F: kernel/fork.c > F: mm/Kconfig > F: mm/debug.c > diff --git a/include/uapi/linux/process_vm.h b/include/uapi/linux/process_vm.h > new file mode 100644 > index 000000000000..4168e09f3f4e > --- /dev/null > +++ b/include/uapi/linux/process_vm.h > @@ -0,0 +1,9 @@ > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > +#ifndef _UAPI_LINUX_PROCESS_VM_H > +#define _UAPI_LINUX_PROCESS_VM_H > + > +/* Flags for process_vm_readv/process_vm_writev */ > +#define PROCESS_VM_PIDFD (1UL << 0) > +#define PROCESS_VM_NOWAIT (1UL << 1) Can we use BIT() here? I see it getting used in other uapi headers. > + > +#endif /* _UAPI_LINUX_PROCESS_VM_H */ > diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c > index 656d3e88755b..b5eac870ef24 100644 > --- a/mm/process_vm_access.c > +++ b/mm/process_vm_access.c > @@ -14,6 +14,7 @@ > #include > #include > #include > +#include > > /** > * process_vm_rw_pages - read/write pages from task specified > @@ -68,6 +69,7 @@ static int process_vm_rw_pages(struct page **pages, > * @mm: mm for task > * @task: task to read/write from > * @vm_write: 0 means copy from, 1 means copy to > + * @pvm_flags: PROCESS_VM_* flags > * Returns 0 on success or on failure error code > */ > static int process_vm_rw_single_vec(unsigned long addr, > @@ -76,7 +78,8 @@ static int process_vm_rw_single_vec(unsigned long addr, > struct page **process_pages, > struct mm_struct *mm, > struct task_struct *task, > - int vm_write) > + int vm_write, > + unsigned int pvm_flags) "unsigned int" but you define the flags as unsigned long above. Applies to all occurrences. We ran into interesting issued with such inconsistency with clone-flags recently :) > { > unsigned long pa = addr & PAGE_MASK; > unsigned long start_offset = addr - pa; > @@ -91,6 +94,8 @@ static int process_vm_rw_single_vec(unsigned long addr, > > if (vm_write) > flags |= FOLL_WRITE; > + if (pvm_flags & PROCESS_VM_NOWAIT) > + flags |= FOLL_NOWAIT; > > while (!rc && nr_pages && iov_iter_count(iter)) { > int pinned_pages = min_t(unsigned long, nr_pages, PVM_MAX_USER_PAGES); > @@ -141,7 +146,7 @@ static int process_vm_rw_single_vec(unsigned long addr, > * @iter: where to copy to/from locally > * @rvec: iovec array specifying where to copy to/from in the other process > * @riovcnt: size of rvec array > - * @flags: currently unused > + * @flags: process_vm_readv/writev flags > * @vm_write: 0 if reading from other process, 1 if writing to other process > * > * Returns the number of bytes read/written or error code. May > @@ -163,6 +168,7 @@ static ssize_t process_vm_rw_core(pid_t pid, struct iov_iter *iter, > unsigned long nr_pages_iov; > ssize_t iov_len; > size_t total_len = iov_iter_count(iter); > + unsigned int f_flags; > > /* > * Work out how many pages of struct pages we're going to need > @@ -194,7 +200,11 @@ static ssize_t process_vm_rw_core(pid_t pid, struct iov_iter *iter, > } > > /* Get process information */ > - task = find_get_task_by_vpid(pid); > + if (flags & PROCESS_VM_PIDFD) > + task = pidfd_get_task(pid, &f_flags); > + else > + task = find_get_task_by_vpid(pid); > + > if (!task) { > rc = -ESRCH; > goto free_proc_pages; > @@ -215,7 +225,7 @@ static ssize_t process_vm_rw_core(pid_t pid, struct iov_iter *iter, > for (i = 0; i < riovcnt && iov_iter_count(iter) && !rc; i++) > rc = process_vm_rw_single_vec( > (unsigned long)rvec[i].iov_base, rvec[i].iov_len, > - iter, process_pages, mm, task, vm_write); > + iter, process_pages, mm, task, vm_write, flags); > > /* copied = space before - space after */ > total_len -= iov_iter_count(iter); > @@ -266,7 +276,7 @@ static ssize_t process_vm_rw(pid_t pid, > ssize_t rc; > int dir = vm_write ? ITER_SOURCE : ITER_DEST; > > - if (flags != 0) > + if (flags & ~(PROCESS_VM_NOWAIT | PROCESS_VM_PIDFD)) Likely we want that wrapped in a new define like #define PROCESS_VM_SUPPORTED_FLAGS (PROCESS_VM_NOWAIT | PROCESS_VM_PIDFD) (likely not in the uapi header but in a kernel-internal one) -- Cheers David