A series of fixes to simplify and improve the GUP interface with an eye to
providing groundwork to future improvements:-
* __access_remote_vm() and access_remote_vm() are functionally identical,
so make the former static such that in future we can potentially change
the external-facing implementation details of this function.
* Extend is_valid_gup_args() to cover the missing FOLL_TOUCH case, and
simplify things by defining INTERNAL_GUP_FLAGS to check against.
* Adjust __get_user_pages_locked() to explicitly treat a failure to pin any
pages as an error in all circumstances other than FOLL_NOWAIT being
specified, bringing it in line with the nommu implementation of this
function.
* (With many thanks to Arnd who suggested this in the first instance)
Update get_user_page_vma_remote() to explicitly only return a page or an
error, simplifying the interface and avoiding the questionable
IS_ERR_OR_NULL() pattern.
v2:
* Corrected spelling mistakes in patches 1 and 2 (thanks PeterZ).
* Made things WARN_ON_ONCE() where appropriate and dropped unnecessary
checks (thanks David).
v1:
https://lore.kernel.org/all/[email protected]/
Lorenzo Stoakes (4):
mm: make __access_remote_vm() static
mm/gup: explicitly define and check internal GUP flags, disallow
FOLL_TOUCH
mm/gup: make failure to pin an error if FOLL_NOWAIT not specified
mm/gup: adapt get_user_page_vma_remote() to never return NULL
arch/arm64/kernel/mte.c | 4 ++--
include/linux/mm.h | 14 +++++++++-----
kernel/events/uprobes.c | 4 ++--
kernel/ptrace.c | 2 +-
mm/gup.c | 16 +++++++++++++---
mm/internal.h | 3 +++
mm/memory.c | 7 +++----
mm/nommu.c | 4 ++--
8 files changed, 35 insertions(+), 19 deletions(-)
--
2.42.0
There really should be no circumstances under which a non-FOLL_NOWAIT GUP
operation fails to return any pages, so make this an error and warn on it.
To catch the trivial case, simply exit early if nr_pages == 0.
This brings __get_user_pages_locked() in line with the behaviour of its
nommu variant.
Reviewed-by: Arnd Bergmann <[email protected]>
Signed-off-by: Lorenzo Stoakes <[email protected]>
---
mm/gup.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/mm/gup.c b/mm/gup.c
index b21b33d1787e..231711efa390 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1471,6 +1471,9 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm,
long ret, pages_done;
bool must_unlock = false;
+ if (!nr_pages)
+ return 0;
+
/*
* The internal caller expects GUP to manage the lock internally and the
* lock must be released when this returns.
@@ -1595,6 +1598,14 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm,
mmap_read_unlock(mm);
*locked = 0;
}
+
+ /*
+ * Failing to pin anything implies something has gone wrong (except when
+ * FOLL_NOWAIT is specified).
+ */
+ if (WARN_ON_ONCE(pages_done == 0 && !(flags & FOLL_NOWAIT)))
+ return -EFAULT;
+
return pages_done;
}
--
2.42.0
get_user_pages_remote() will never return 0 except in the case of
FOLL_NOWAIT being specified, which we explicitly disallow.
This simplifies error handling for the caller and avoids the awkwardness of
dealing with both errors and failing to pin. Failing to pin here is an
error.
Suggested-by: Arnd Bergmann <[email protected]>
Reviewed-by: Arnd Bergmann <[email protected]>
Acked-by: Catalin Marinas <[email protected]>
Signed-off-by: Lorenzo Stoakes <[email protected]>
---
arch/arm64/kernel/mte.c | 4 ++--
include/linux/mm.h | 12 +++++++++---
kernel/events/uprobes.c | 4 ++--
mm/memory.c | 3 +--
4 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index 4edecaac8f91..8878b392df58 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -411,8 +411,8 @@ static int __access_remote_tags(struct mm_struct *mm, unsigned long addr,
struct page *page = get_user_page_vma_remote(mm, addr,
gup_flags, &vma);
- if (IS_ERR_OR_NULL(page)) {
- err = page == NULL ? -EIO : PTR_ERR(page);
+ if (IS_ERR(page)) {
+ err = PTR_ERR(page);
break;
}
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 7b89f7bd420d..fa608cba041f 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2425,6 +2425,9 @@ long pin_user_pages_remote(struct mm_struct *mm,
unsigned int gup_flags, struct page **pages,
int *locked);
+/*
+ * Retrieves a single page alongside its VMA. Does not support FOLL_NOWAIT.
+ */
static inline struct page *get_user_page_vma_remote(struct mm_struct *mm,
unsigned long addr,
int gup_flags,
@@ -2432,12 +2435,15 @@ static inline struct page *get_user_page_vma_remote(struct mm_struct *mm,
{
struct page *page;
struct vm_area_struct *vma;
- int got = get_user_pages_remote(mm, addr, 1, gup_flags, &page, NULL);
+ int got;
+
+ if (WARN_ON_ONCE(unlikely(gup_flags & FOLL_NOWAIT)))
+ return ERR_PTR(-EINVAL);
+
+ got = get_user_pages_remote(mm, addr, 1, gup_flags, &page, NULL);
if (got < 0)
return ERR_PTR(got);
- if (got == 0)
- return NULL;
vma = vma_lookup(mm, addr);
if (WARN_ON_ONCE(!vma)) {
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 3048589e2e85..435aac1d8c27 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -474,8 +474,8 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
gup_flags |= FOLL_SPLIT_PMD;
/* Read the page with vaddr into memory */
old_page = get_user_page_vma_remote(mm, vaddr, gup_flags, &vma);
- if (IS_ERR_OR_NULL(old_page))
- return old_page ? PTR_ERR(old_page) : 0;
+ if (IS_ERR(old_page))
+ return PTR_ERR(old_page);
ret = verify_opcode(old_page, vaddr, &opcode);
if (ret <= 0)
diff --git a/mm/memory.c b/mm/memory.c
index e2743aa95b56..f2eef3d1cf58 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5905,7 +5905,7 @@ static int __access_remote_vm(struct mm_struct *mm, unsigned long addr,
struct page *page = get_user_page_vma_remote(mm, addr,
gup_flags, &vma);
- if (IS_ERR_OR_NULL(page)) {
+ if (IS_ERR(page)) {
/* We might need to expand the stack to access it */
vma = vma_lookup(mm, addr);
if (!vma) {
@@ -5919,7 +5919,6 @@ static int __access_remote_vm(struct mm_struct *mm, unsigned long addr,
continue;
}
-
/*
* Check if this is a VM_IO | VM_PFNMAP VMA, which
* we can access using slightly different code.
--
2.42.0
On 03.10.23 01:14, Lorenzo Stoakes wrote:
> There really should be no circumstances under which a non-FOLL_NOWAIT GUP
> operation fails to return any pages, so make this an error and warn on it.
>
> To catch the trivial case, simply exit early if nr_pages == 0.
>
> This brings __get_user_pages_locked() in line with the behaviour of its
> nommu variant.
>
> Reviewed-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Lorenzo Stoakes <[email protected]>
> ---
Reviewed-by: David Hildenbrand <[email protected]>
--
Cheers,
David / dhildenb
On 03.10.23 01:14, Lorenzo Stoakes wrote:
> get_user_pages_remote() will never return 0 except in the case of
> FOLL_NOWAIT being specified, which we explicitly disallow.
>
> This simplifies error handling for the caller and avoids the awkwardness of
> dealing with both errors and failing to pin. Failing to pin here is an
> error.
>
> Suggested-by: Arnd Bergmann <[email protected]>
> Reviewed-by: Arnd Bergmann <[email protected]>
> Acked-by: Catalin Marinas <[email protected]>
> Signed-off-by: Lorenzo Stoakes <[email protected]>
> ---
Reviewed-by: David Hildenbrand <[email protected]>
--
Cheers,
David / dhildenb