* [PATCH 0/7] mm/gup: Minor fix, cleanup and improvements
@ 2025-03-30 12:17 Baoquan He
2025-03-30 12:17 ` [PATCH 1/7] mm/gup: fix wrongly calculated returned value in fault_in_safe_writeable() Baoquan He
` (6 more replies)
0 siblings, 7 replies; 16+ messages in thread
From: Baoquan He @ 2025-03-30 12:17 UTC (permalink / raw)
To: akpm; +Cc: linux-mm, linux-kernel, Baoquan He
These are made when I explore codes in mm/gup.c.
Baoquan He (7):
mm/gup: fix wrongly calculated returned value in
fault_in_safe_writeable()
mm/gup: check if both GUP_GET and GUP_PIN are set in
__get_user_pages() earlier
mm/gup: Fix the outdated code comments above get_user_pages_unlocked()
mm/gup: remove gup_fast_pgd_leaf() and clean up the relevant codes
x86/mm: remove pgd_leaf definition in arch
x86/mm: remove p4d_leaf definition
mm/pgtable: remove unneeded pgd_devmap()
arch/arm64/include/asm/pgtable.h | 5 --
arch/loongarch/include/asm/pgtable.h | 1 -
arch/powerpc/include/asm/book3s/64/pgtable.h | 5 --
arch/riscv/include/asm/pgtable-64.h | 5 --
arch/x86/include/asm/pgtable.h | 15 ----
arch/x86/mm/pti.c | 4 +-
include/linux/pgtable.h | 4 -
mm/gup.c | 79 ++++----------------
8 files changed, 15 insertions(+), 103 deletions(-)
--
2.41.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/7] mm/gup: fix wrongly calculated returned value in fault_in_safe_writeable()
2025-03-30 12:17 [PATCH 0/7] mm/gup: Minor fix, cleanup and improvements Baoquan He
@ 2025-03-30 12:17 ` Baoquan He
2025-03-30 19:43 ` Zhu Yanjun
2025-03-30 12:17 ` [PATCH 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set in __get_user_pages() earlier Baoquan He
` (5 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Baoquan He @ 2025-03-30 12:17 UTC (permalink / raw)
To: akpm; +Cc: linux-mm, linux-kernel, Baoquan He
Not like fault_in_writeable() or fault_in_writeable(), in
fault_in_safe_writeable() local variable 'start' is increased page
by page to loop till the whole address range is handled. However,
it mistakenly calcalates the size of handled range with 'uaddr - start'.
Fix it here.
Signed-off-by: Baoquan He <bhe@redhat.com>
---
mm/gup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/gup.c b/mm/gup.c
index 855ab860f88b..73777b1de679 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2207,8 +2207,8 @@ size_t fault_in_safe_writeable(const char __user *uaddr, size_t size)
} while (start != end);
mmap_read_unlock(mm);
- if (size > (unsigned long)uaddr - start)
- return size - ((unsigned long)uaddr - start);
+ if (size > start - (unsigned long)uaddr)
+ return size - (start - (unsigned long)uaddr);
return 0;
}
EXPORT_SYMBOL(fault_in_safe_writeable);
--
2.41.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set in __get_user_pages() earlier
2025-03-30 12:17 [PATCH 0/7] mm/gup: Minor fix, cleanup and improvements Baoquan He
2025-03-30 12:17 ` [PATCH 1/7] mm/gup: fix wrongly calculated returned value in fault_in_safe_writeable() Baoquan He
@ 2025-03-30 12:17 ` Baoquan He
2025-03-30 13:49 ` kernel test robot
` (2 more replies)
2025-03-30 12:17 ` [PATCH 3/7] mm/gup: Fix the outdated code comments above get_user_pages_unlocked() Baoquan He
` (4 subsequent siblings)
6 siblings, 3 replies; 16+ messages in thread
From: Baoquan He @ 2025-03-30 12:17 UTC (permalink / raw)
To: akpm; +Cc: linux-mm, linux-kernel, Baoquan He
In __get_user_pages(), it will traverse page table and take a reference
to the page the given user address corresponds to if GUP_GET or GUP_PIN
is et. However, it's not supported both GUP_GET and GUP_PIN are set.
This check should be done earlier, but not doing it till entering into
follow_page_pte() and failed.
Here move the checking to the beginning of __get_user_pages().
Signed-off-by: Baoquan He <bhe@redhat.com>
---
mm/gup.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/mm/gup.c b/mm/gup.c
index 73777b1de679..8788105daee8 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -847,11 +847,6 @@ static struct page *follow_page_pte(struct vm_area_struct *vma,
pte_t *ptep, pte;
int ret;
- /* FOLL_GET and FOLL_PIN are mutually exclusive. */
- if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) ==
- (FOLL_PIN | FOLL_GET)))
- return ERR_PTR(-EINVAL);
-
ptep = pte_offset_map_lock(mm, pmd, address, &ptl);
if (!ptep)
return no_page_table(vma, flags, address);
@@ -1434,6 +1429,11 @@ static long __get_user_pages(struct mm_struct *mm,
VM_BUG_ON(!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN)));
+ /* FOLL_GET and FOLL_PIN are mutually exclusive. */
+ if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) ==
+ (FOLL_PIN | FOLL_GET)))
+ return ERR_PTR(-EINVAL);
+
do {
struct page *page;
unsigned int page_increm;
--
2.41.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/7] mm/gup: Fix the outdated code comments above get_user_pages_unlocked()
2025-03-30 12:17 [PATCH 0/7] mm/gup: Minor fix, cleanup and improvements Baoquan He
2025-03-30 12:17 ` [PATCH 1/7] mm/gup: fix wrongly calculated returned value in fault_in_safe_writeable() Baoquan He
2025-03-30 12:17 ` [PATCH 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set in __get_user_pages() earlier Baoquan He
@ 2025-03-30 12:17 ` Baoquan He
2025-03-31 13:58 ` Oscar Salvador
2025-03-30 12:17 ` [PATCH 4/7] mm/gup: remove gup_fast_pgd_leaf() and clean up the relevant codes Baoquan He
` (3 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Baoquan He @ 2025-03-30 12:17 UTC (permalink / raw)
To: akpm; +Cc: linux-mm, linux-kernel, Baoquan He
Since commit f0818f472d8d ("mm: gup: add get_user_pages_locked
and get_user_pages_unlocked"), get_user_pages() doesn't need to have
mmap_lock held anymore. It calls __get_user_pages_locked() which
can acquire and drop the mmap_lock internaly.
Hence remove the incorrect code comments now.
Signed-off-by: Baoquan He <bhe@redhat.com>
---
mm/gup.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/mm/gup.c b/mm/gup.c
index 8788105daee8..3345a065c2cb 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2702,19 +2702,9 @@ long get_user_pages(unsigned long start, unsigned long nr_pages,
EXPORT_SYMBOL(get_user_pages);
/*
- * get_user_pages_unlocked() is suitable to replace the form:
- *
- * mmap_read_lock(mm);
- * get_user_pages(mm, ..., pages, NULL);
- * mmap_read_unlock(mm);
- *
- * with:
- *
- * get_user_pages_unlocked(mm, ..., pages);
- *
- * It is functionally equivalent to get_user_pages_fast so
- * get_user_pages_fast should be used instead if specific gup_flags
- * (e.g. FOLL_FORCE) are not required.
+ * get_user_pages_unlocked() is functionally equivalent to
+ * get_user_pages_fast so get_user_pages_fast should be used instead
+ * if specific gup_flags (e.g. FOLL_FORCE) are not required.
*/
long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages,
struct page **pages, unsigned int gup_flags)
--
2.41.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 4/7] mm/gup: remove gup_fast_pgd_leaf() and clean up the relevant codes
2025-03-30 12:17 [PATCH 0/7] mm/gup: Minor fix, cleanup and improvements Baoquan He
` (2 preceding siblings ...)
2025-03-30 12:17 ` [PATCH 3/7] mm/gup: Fix the outdated code comments above get_user_pages_unlocked() Baoquan He
@ 2025-03-30 12:17 ` Baoquan He
2025-03-30 12:17 ` [PATCH 5/7] x86/mm: remove pgd_leaf definition in arch Baoquan He
` (2 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Baoquan He @ 2025-03-30 12:17 UTC (permalink / raw)
To: akpm; +Cc: linux-mm, linux-kernel, Baoquan He
In the current kernel, only pud huge page is supported in some
architectures. P4d and pgd huge pages haven't been supported yet.
And in mm/gup.c, there's no pgd huge page handling in the
follow_page_mask() code path. Hence it doesn't make sense to have
gup_fast_pgd_leaf() in gup_fast code path.
Here remove gup_fast_pgd_leaf() and clean up the relevant codes.
Signed-off-by: Baoquan He <bhe@redhat.com>
---
mm/gup.c | 49 +++----------------------------------------------
1 file changed, 3 insertions(+), 46 deletions(-)
diff --git a/mm/gup.c b/mm/gup.c
index 3345a065c2cb..3d2c57f59b4d 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -3168,46 +3168,6 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,
return 1;
}
-static int gup_fast_pgd_leaf(pgd_t orig, pgd_t *pgdp, unsigned long addr,
- unsigned long end, unsigned int flags, struct page **pages,
- int *nr)
-{
- int refs;
- struct page *page;
- struct folio *folio;
-
- if (!pgd_access_permitted(orig, flags & FOLL_WRITE))
- return 0;
-
- BUILD_BUG_ON(pgd_devmap(orig));
-
- page = pgd_page(orig);
- refs = record_subpages(page, PGDIR_SIZE, addr, end, pages + *nr);
-
- folio = try_grab_folio_fast(page, refs, flags);
- if (!folio)
- return 0;
-
- if (unlikely(pgd_val(orig) != pgd_val(*pgdp))) {
- gup_put_folio(folio, refs, flags);
- return 0;
- }
-
- if (!pgd_write(orig) && gup_must_unshare(NULL, flags, &folio->page)) {
- gup_put_folio(folio, refs, flags);
- return 0;
- }
-
- if (!gup_fast_folio_allowed(folio, flags)) {
- gup_put_folio(folio, refs, flags);
- return 0;
- }
-
- *nr += refs;
- folio_set_referenced(folio);
- return 1;
-}
-
static int gup_fast_pmd_range(pud_t *pudp, pud_t pud, unsigned long addr,
unsigned long end, unsigned int flags, struct page **pages,
int *nr)
@@ -3302,12 +3262,9 @@ static void gup_fast_pgd_range(unsigned long addr, unsigned long end,
next = pgd_addr_end(addr, end);
if (pgd_none(pgd))
return;
- if (unlikely(pgd_leaf(pgd))) {
- if (!gup_fast_pgd_leaf(pgd, pgdp, addr, next, flags,
- pages, nr))
- return;
- } else if (!gup_fast_p4d_range(pgdp, pgd, addr, next, flags,
- pages, nr))
+ BUILD_BUG_ON(pgd_leaf(pgd));
+ if (!gup_fast_p4d_range(pgdp, pgd, addr, next, flags,
+ pages, nr))
return;
} while (pgdp++, addr = next, addr != end);
}
--
2.41.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 5/7] x86/mm: remove pgd_leaf definition in arch
2025-03-30 12:17 [PATCH 0/7] mm/gup: Minor fix, cleanup and improvements Baoquan He
` (3 preceding siblings ...)
2025-03-30 12:17 ` [PATCH 4/7] mm/gup: remove gup_fast_pgd_leaf() and clean up the relevant codes Baoquan He
@ 2025-03-30 12:17 ` Baoquan He
2025-03-30 12:17 ` [PATCH 6/7] x86/mm: remove p4d_leaf definition Baoquan He
2025-03-30 12:17 ` [PATCH 7/7] mm/pgtable: remove unneeded pgd_devmap() Baoquan He
6 siblings, 0 replies; 16+ messages in thread
From: Baoquan He @ 2025-03-30 12:17 UTC (permalink / raw)
To: akpm; +Cc: linux-mm, linux-kernel, Baoquan He
pgd huge page is not supported yet, let's use the generic definition
in linux/pgtable.h.
And also update the BUILD_BUG_ON() checking for pgd_leaf() in
pti_user_pagetable_walk_p4d() because pgd_leaf() returns boolean value.
Signed-off-by: Baoquan He <bhe@redhat.com>
---
arch/x86/include/asm/pgtable.h | 3 ---
arch/x86/mm/pti.c | 2 +-
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 7bd6bd6df4a1..5f4fcc0eea17 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -1472,9 +1472,6 @@ static inline bool pgdp_maps_userspace(void *__ptr)
return (((ptr & ~PAGE_MASK) / sizeof(pgd_t)) < PGD_KERNEL_START);
}
-#define pgd_leaf pgd_leaf
-static inline bool pgd_leaf(pgd_t pgd) { return false; }
-
#ifdef CONFIG_MITIGATION_PAGE_TABLE_ISOLATION
/*
* All top-level MITIGATION_PAGE_TABLE_ISOLATION page tables are order-1 pages
diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index 5f0d579932c6..c2e1de40136f 100644
--- a/arch/x86/mm/pti.c
+++ b/arch/x86/mm/pti.c
@@ -185,7 +185,7 @@ static p4d_t *pti_user_pagetable_walk_p4d(unsigned long address)
set_pgd(pgd, __pgd(_KERNPG_TABLE | __pa(new_p4d_page)));
}
- BUILD_BUG_ON(pgd_leaf(*pgd) != 0);
+ BUILD_BUG_ON(pgd_leaf(*pgd));
return p4d_offset(pgd, address);
}
--
2.41.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 6/7] x86/mm: remove p4d_leaf definition
2025-03-30 12:17 [PATCH 0/7] mm/gup: Minor fix, cleanup and improvements Baoquan He
` (4 preceding siblings ...)
2025-03-30 12:17 ` [PATCH 5/7] x86/mm: remove pgd_leaf definition in arch Baoquan He
@ 2025-03-30 12:17 ` Baoquan He
2025-03-30 12:17 ` [PATCH 7/7] mm/pgtable: remove unneeded pgd_devmap() Baoquan He
6 siblings, 0 replies; 16+ messages in thread
From: Baoquan He @ 2025-03-30 12:17 UTC (permalink / raw)
To: akpm; +Cc: linux-mm, linux-kernel, Baoquan He
There's no p4d huge page support yet, let's use the generic definition.
And also update the BUILD_BUG_ON() in pti_user_pagetable_walk_pmd()
because p4d_leaf() returns boolean value.
Signed-off-by: Baoquan He <bhe@redhat.com>
---
arch/x86/include/asm/pgtable.h | 7 -------
arch/x86/mm/pti.c | 2 +-
2 files changed, 1 insertion(+), 8 deletions(-)
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 5f4fcc0eea17..5ddba366d3b4 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -292,13 +292,6 @@ static inline unsigned long pgd_pfn(pgd_t pgd)
return (pgd_val(pgd) & PTE_PFN_MASK) >> PAGE_SHIFT;
}
-#define p4d_leaf p4d_leaf
-static inline bool p4d_leaf(p4d_t p4d)
-{
- /* No 512 GiB pages yet */
- return 0;
-}
-
#define pte_page(pte) pfn_to_page(pte_pfn(pte))
#define pmd_leaf pmd_leaf
diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index c2e1de40136f..190299834011 100644
--- a/arch/x86/mm/pti.c
+++ b/arch/x86/mm/pti.c
@@ -206,7 +206,7 @@ static pmd_t *pti_user_pagetable_walk_pmd(unsigned long address)
if (!p4d)
return NULL;
- BUILD_BUG_ON(p4d_leaf(*p4d) != 0);
+ BUILD_BUG_ON(p4d_leaf(*p4d));
if (p4d_none(*p4d)) {
unsigned long new_pud_page = __get_free_page(gfp);
if (WARN_ON_ONCE(!new_pud_page))
--
2.41.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 7/7] mm/pgtable: remove unneeded pgd_devmap()
2025-03-30 12:17 [PATCH 0/7] mm/gup: Minor fix, cleanup and improvements Baoquan He
` (5 preceding siblings ...)
2025-03-30 12:17 ` [PATCH 6/7] x86/mm: remove p4d_leaf definition Baoquan He
@ 2025-03-30 12:17 ` Baoquan He
6 siblings, 0 replies; 16+ messages in thread
From: Baoquan He @ 2025-03-30 12:17 UTC (permalink / raw)
To: akpm; +Cc: linux-mm, linux-kernel, Baoquan He
There's no user of pgd_devmap() now, remove it from all ARCH-es
and linux/pgtable.h.
Signed-off-by: Baoquan He <bhe@redhat.com>
---
arch/arm64/include/asm/pgtable.h | 5 -----
arch/loongarch/include/asm/pgtable.h | 1 -
arch/powerpc/include/asm/book3s/64/pgtable.h | 5 -----
arch/riscv/include/asm/pgtable-64.h | 5 -----
arch/x86/include/asm/pgtable.h | 5 -----
include/linux/pgtable.h | 4 ----
6 files changed, 25 deletions(-)
diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index 84f05f781a70..e0cab581edc9 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -1230,11 +1230,6 @@ static inline int pud_devmap(pud_t pud)
{
return 0;
}
-
-static inline int pgd_devmap(pgd_t pgd)
-{
- return 0;
-}
#endif
#ifdef CONFIG_PAGE_TABLE_CHECK
diff --git a/arch/loongarch/include/asm/pgtable.h b/arch/loongarch/include/asm/pgtable.h
index da346733a1da..d9b04296d9f5 100644
--- a/arch/loongarch/include/asm/pgtable.h
+++ b/arch/loongarch/include/asm/pgtable.h
@@ -615,7 +615,6 @@ static inline long pmd_protnone(pmd_t pmd)
#ifdef CONFIG_TRANSPARENT_HUGEPAGE
#define pud_devmap(pud) (0)
-#define pgd_devmap(pgd) (0)
#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
/*
diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 6d98e6f08d4d..0da1c8d7f778 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -1405,11 +1405,6 @@ static inline int pud_devmap(pud_t pud)
{
return pte_devmap(pud_pte(pud));
}
-
-static inline int pgd_devmap(pgd_t pgd)
-{
- return 0;
-}
#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
#define __HAVE_ARCH_PTEP_MODIFY_PROT_TRANSACTION
diff --git a/arch/riscv/include/asm/pgtable-64.h b/arch/riscv/include/asm/pgtable-64.h
index 0897dd99ab8d..c6da4c8354a3 100644
--- a/arch/riscv/include/asm/pgtable-64.h
+++ b/arch/riscv/include/asm/pgtable-64.h
@@ -411,11 +411,6 @@ static inline int pud_devmap(pud_t pud)
{
return 0;
}
-
-static inline int pgd_devmap(pgd_t pgd)
-{
- return 0;
-}
#endif
#endif /* _ASM_RISCV_PGTABLE_64_H */
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 5ddba366d3b4..3d6e78af525a 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -361,11 +361,6 @@ static inline pud_t pud_mkspecial(pud_t pud)
return pud_set_flags(pud, _PAGE_SPECIAL);
}
#endif /* CONFIG_ARCH_SUPPORTS_PUD_PFNMAP */
-
-static inline int pgd_devmap(pgd_t pgd)
-{
- return 0;
-}
#endif
#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
index 94d267d02372..9d2dd5f71443 100644
--- a/include/linux/pgtable.h
+++ b/include/linux/pgtable.h
@@ -1615,10 +1615,6 @@ static inline int pud_devmap(pud_t pud)
{
return 0;
}
-static inline int pgd_devmap(pgd_t pgd)
-{
- return 0;
-}
#endif
#if !defined(CONFIG_TRANSPARENT_HUGEPAGE) || \
--
2.41.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set in __get_user_pages() earlier
2025-03-30 12:17 ` [PATCH 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set in __get_user_pages() earlier Baoquan He
@ 2025-03-30 13:49 ` kernel test robot
2025-03-30 14:19 ` Baoquan He
2025-03-30 14:09 ` kernel test robot
2025-03-30 14:20 ` kernel test robot
2 siblings, 1 reply; 16+ messages in thread
From: kernel test robot @ 2025-03-30 13:49 UTC (permalink / raw)
To: Baoquan He, akpm; +Cc: oe-kbuild-all, linux-mm, linux-kernel, Baoquan He
Hi Baoquan,
kernel test robot noticed the following build warnings:
[auto build test WARNING on akpm-mm/mm-everything]
url: https://github.com/intel-lab-lkp/linux/commits/Baoquan-He/mm-gup-fix-wrongly-calculated-returned-value-in-fault_in_safe_writeable/20250330-201949
base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link: https://lore.kernel.org/r/20250330121718.175815-3-bhe%40redhat.com
patch subject: [PATCH 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set in __get_user_pages() earlier
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20250330/202503302151.MdrisJhx-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250330/202503302151.MdrisJhx-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503302151.MdrisJhx-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from arch/x86/include/asm/bug.h:110,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from include/linux/spinlock.h:60,
from mm/gup.c:5:
mm/gup.c: In function '__get_user_pages':
mm/gup.c:1433:27: error: 'flags' undeclared (first use in this function)
1433 | if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) ==
| ^~~~~
include/asm-generic/bug.h:121:32: note: in definition of macro 'WARN_ON_ONCE'
121 | int __ret_warn_on = !!(condition); \
| ^~~~~~~~~
mm/gup.c:1433:27: note: each undeclared identifier is reported only once for each function it appears in
1433 | if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) ==
| ^~~~~
include/asm-generic/bug.h:121:32: note: in definition of macro 'WARN_ON_ONCE'
121 | int __ret_warn_on = !!(condition); \
| ^~~~~~~~~
>> mm/gup.c:1435:24: warning: returning 'void *' from a function with return type 'long int' makes integer from pointer without a cast [-Wint-conversion]
1435 | return ERR_PTR(-EINVAL);
| ^~~~~~~~~~~~~~~~
vim +1435 mm/gup.c
1361
1362 /**
1363 * __get_user_pages() - pin user pages in memory
1364 * @mm: mm_struct of target mm
1365 * @start: starting user address
1366 * @nr_pages: number of pages from start to pin
1367 * @gup_flags: flags modifying pin behaviour
1368 * @pages: array that receives pointers to the pages pinned.
1369 * Should be at least nr_pages long. Or NULL, if caller
1370 * only intends to ensure the pages are faulted in.
1371 * @locked: whether we're still with the mmap_lock held
1372 *
1373 * Returns either number of pages pinned (which may be less than the
1374 * number requested), or an error. Details about the return value:
1375 *
1376 * -- If nr_pages is 0, returns 0.
1377 * -- If nr_pages is >0, but no pages were pinned, returns -errno.
1378 * -- If nr_pages is >0, and some pages were pinned, returns the number of
1379 * pages pinned. Again, this may be less than nr_pages.
1380 * -- 0 return value is possible when the fault would need to be retried.
1381 *
1382 * The caller is responsible for releasing returned @pages, via put_page().
1383 *
1384 * Must be called with mmap_lock held. It may be released. See below.
1385 *
1386 * __get_user_pages walks a process's page tables and takes a reference to
1387 * each struct page that each user address corresponds to at a given
1388 * instant. That is, it takes the page that would be accessed if a user
1389 * thread accesses the given user virtual address at that instant.
1390 *
1391 * This does not guarantee that the page exists in the user mappings when
1392 * __get_user_pages returns, and there may even be a completely different
1393 * page there in some cases (eg. if mmapped pagecache has been invalidated
1394 * and subsequently re-faulted). However it does guarantee that the page
1395 * won't be freed completely. And mostly callers simply care that the page
1396 * contains data that was valid *at some point in time*. Typically, an IO
1397 * or similar operation cannot guarantee anything stronger anyway because
1398 * locks can't be held over the syscall boundary.
1399 *
1400 * If @gup_flags & FOLL_WRITE == 0, the page must not be written to. If
1401 * the page is written to, set_page_dirty (or set_page_dirty_lock, as
1402 * appropriate) must be called after the page is finished with, and
1403 * before put_page is called.
1404 *
1405 * If FOLL_UNLOCKABLE is set without FOLL_NOWAIT then the mmap_lock may
1406 * be released. If this happens *@locked will be set to 0 on return.
1407 *
1408 * A caller using such a combination of @gup_flags must therefore hold the
1409 * mmap_lock for reading only, and recognize when it's been released. Otherwise,
1410 * it must be held for either reading or writing and will not be released.
1411 *
1412 * In most cases, get_user_pages or get_user_pages_fast should be used
1413 * instead of __get_user_pages. __get_user_pages should be used only if
1414 * you need some special @gup_flags.
1415 */
1416 static long __get_user_pages(struct mm_struct *mm,
1417 unsigned long start, unsigned long nr_pages,
1418 unsigned int gup_flags, struct page **pages,
1419 int *locked)
1420 {
1421 long ret = 0, i = 0;
1422 struct vm_area_struct *vma = NULL;
1423 struct follow_page_context ctx = { NULL };
1424
1425 if (!nr_pages)
1426 return 0;
1427
1428 start = untagged_addr_remote(mm, start);
1429
1430 VM_BUG_ON(!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN)));
1431
1432 /* FOLL_GET and FOLL_PIN are mutually exclusive. */
1433 if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) ==
1434 (FOLL_PIN | FOLL_GET)))
> 1435 return ERR_PTR(-EINVAL);
1436
1437 do {
1438 struct page *page;
1439 unsigned int page_increm;
1440
1441 /* first iteration or cross vma bound */
1442 if (!vma || start >= vma->vm_end) {
1443 /*
1444 * MADV_POPULATE_(READ|WRITE) wants to handle VMA
1445 * lookups+error reporting differently.
1446 */
1447 if (gup_flags & FOLL_MADV_POPULATE) {
1448 vma = vma_lookup(mm, start);
1449 if (!vma) {
1450 ret = -ENOMEM;
1451 goto out;
1452 }
1453 if (check_vma_flags(vma, gup_flags)) {
1454 ret = -EINVAL;
1455 goto out;
1456 }
1457 goto retry;
1458 }
1459 vma = gup_vma_lookup(mm, start);
1460 if (!vma && in_gate_area(mm, start)) {
1461 ret = get_gate_page(mm, start & PAGE_MASK,
1462 gup_flags, &vma,
1463 pages ? &page : NULL);
1464 if (ret)
1465 goto out;
1466 ctx.page_mask = 0;
1467 goto next_page;
1468 }
1469
1470 if (!vma) {
1471 ret = -EFAULT;
1472 goto out;
1473 }
1474 ret = check_vma_flags(vma, gup_flags);
1475 if (ret)
1476 goto out;
1477 }
1478 retry:
1479 /*
1480 * If we have a pending SIGKILL, don't keep faulting pages and
1481 * potentially allocating memory.
1482 */
1483 if (fatal_signal_pending(current)) {
1484 ret = -EINTR;
1485 goto out;
1486 }
1487 cond_resched();
1488
1489 page = follow_page_mask(vma, start, gup_flags, &ctx);
1490 if (!page || PTR_ERR(page) == -EMLINK) {
1491 ret = faultin_page(vma, start, gup_flags,
1492 PTR_ERR(page) == -EMLINK, locked);
1493 switch (ret) {
1494 case 0:
1495 goto retry;
1496 case -EBUSY:
1497 case -EAGAIN:
1498 ret = 0;
1499 fallthrough;
1500 case -EFAULT:
1501 case -ENOMEM:
1502 case -EHWPOISON:
1503 goto out;
1504 }
1505 BUG();
1506 } else if (PTR_ERR(page) == -EEXIST) {
1507 /*
1508 * Proper page table entry exists, but no corresponding
1509 * struct page. If the caller expects **pages to be
1510 * filled in, bail out now, because that can't be done
1511 * for this page.
1512 */
1513 if (pages) {
1514 ret = PTR_ERR(page);
1515 goto out;
1516 }
1517 } else if (IS_ERR(page)) {
1518 ret = PTR_ERR(page);
1519 goto out;
1520 }
1521 next_page:
1522 page_increm = 1 + (~(start >> PAGE_SHIFT) & ctx.page_mask);
1523 if (page_increm > nr_pages)
1524 page_increm = nr_pages;
1525
1526 if (pages) {
1527 struct page *subpage;
1528 unsigned int j;
1529
1530 /*
1531 * This must be a large folio (and doesn't need to
1532 * be the whole folio; it can be part of it), do
1533 * the refcount work for all the subpages too.
1534 *
1535 * NOTE: here the page may not be the head page
1536 * e.g. when start addr is not thp-size aligned.
1537 * try_grab_folio() should have taken care of tail
1538 * pages.
1539 */
1540 if (page_increm > 1) {
1541 struct folio *folio = page_folio(page);
1542
1543 /*
1544 * Since we already hold refcount on the
1545 * large folio, this should never fail.
1546 */
1547 if (try_grab_folio(folio, page_increm - 1,
1548 gup_flags)) {
1549 /*
1550 * Release the 1st page ref if the
1551 * folio is problematic, fail hard.
1552 */
1553 gup_put_folio(folio, 1, gup_flags);
1554 ret = -EFAULT;
1555 goto out;
1556 }
1557 }
1558
1559 for (j = 0; j < page_increm; j++) {
1560 subpage = nth_page(page, j);
1561 pages[i + j] = subpage;
1562 flush_anon_page(vma, subpage, start + j * PAGE_SIZE);
1563 flush_dcache_page(subpage);
1564 }
1565 }
1566
1567 i += page_increm;
1568 start += page_increm * PAGE_SIZE;
1569 nr_pages -= page_increm;
1570 } while (nr_pages);
1571 out:
1572 if (ctx.pgmap)
1573 put_dev_pagemap(ctx.pgmap);
1574 return i ? i : ret;
1575 }
1576
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set in __get_user_pages() earlier
2025-03-30 12:17 ` [PATCH 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set in __get_user_pages() earlier Baoquan He
2025-03-30 13:49 ` kernel test robot
@ 2025-03-30 14:09 ` kernel test robot
2025-03-30 14:20 ` kernel test robot
2 siblings, 0 replies; 16+ messages in thread
From: kernel test robot @ 2025-03-30 14:09 UTC (permalink / raw)
To: Baoquan He, akpm; +Cc: llvm, oe-kbuild-all, linux-mm, linux-kernel, Baoquan He
Hi Baoquan,
kernel test robot noticed the following build errors:
[auto build test ERROR on akpm-mm/mm-everything]
url: https://github.com/intel-lab-lkp/linux/commits/Baoquan-He/mm-gup-fix-wrongly-calculated-returned-value-in-fault_in_safe_writeable/20250330-201949
base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link: https://lore.kernel.org/r/20250330121718.175815-3-bhe%40redhat.com
patch subject: [PATCH 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set in __get_user_pages() earlier
config: x86_64-allnoconfig (https://download.01.org/0day-ci/archive/20250330/202503302116.cBgHEPWk-lkp@intel.com/config)
compiler: clang version 20.1.1 (https://github.com/llvm/llvm-project 424c2d9b7e4de40d0804dd374721e6411c27d1d1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250330/202503302116.cBgHEPWk-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503302116.cBgHEPWk-lkp@intel.com/
All errors (new ones prefixed by >>):
>> mm/gup.c:1433:20: error: use of undeclared identifier 'flags'
1433 | if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) ==
| ^
>> mm/gup.c:1435:10: error: incompatible pointer to integer conversion returning 'void *' from a function with result type 'long' [-Wint-conversion]
1435 | return ERR_PTR(-EINVAL);
| ^~~~~~~~~~~~~~~~
2 errors generated.
vim +/flags +1433 mm/gup.c
1361
1362 /**
1363 * __get_user_pages() - pin user pages in memory
1364 * @mm: mm_struct of target mm
1365 * @start: starting user address
1366 * @nr_pages: number of pages from start to pin
1367 * @gup_flags: flags modifying pin behaviour
1368 * @pages: array that receives pointers to the pages pinned.
1369 * Should be at least nr_pages long. Or NULL, if caller
1370 * only intends to ensure the pages are faulted in.
1371 * @locked: whether we're still with the mmap_lock held
1372 *
1373 * Returns either number of pages pinned (which may be less than the
1374 * number requested), or an error. Details about the return value:
1375 *
1376 * -- If nr_pages is 0, returns 0.
1377 * -- If nr_pages is >0, but no pages were pinned, returns -errno.
1378 * -- If nr_pages is >0, and some pages were pinned, returns the number of
1379 * pages pinned. Again, this may be less than nr_pages.
1380 * -- 0 return value is possible when the fault would need to be retried.
1381 *
1382 * The caller is responsible for releasing returned @pages, via put_page().
1383 *
1384 * Must be called with mmap_lock held. It may be released. See below.
1385 *
1386 * __get_user_pages walks a process's page tables and takes a reference to
1387 * each struct page that each user address corresponds to at a given
1388 * instant. That is, it takes the page that would be accessed if a user
1389 * thread accesses the given user virtual address at that instant.
1390 *
1391 * This does not guarantee that the page exists in the user mappings when
1392 * __get_user_pages returns, and there may even be a completely different
1393 * page there in some cases (eg. if mmapped pagecache has been invalidated
1394 * and subsequently re-faulted). However it does guarantee that the page
1395 * won't be freed completely. And mostly callers simply care that the page
1396 * contains data that was valid *at some point in time*. Typically, an IO
1397 * or similar operation cannot guarantee anything stronger anyway because
1398 * locks can't be held over the syscall boundary.
1399 *
1400 * If @gup_flags & FOLL_WRITE == 0, the page must not be written to. If
1401 * the page is written to, set_page_dirty (or set_page_dirty_lock, as
1402 * appropriate) must be called after the page is finished with, and
1403 * before put_page is called.
1404 *
1405 * If FOLL_UNLOCKABLE is set without FOLL_NOWAIT then the mmap_lock may
1406 * be released. If this happens *@locked will be set to 0 on return.
1407 *
1408 * A caller using such a combination of @gup_flags must therefore hold the
1409 * mmap_lock for reading only, and recognize when it's been released. Otherwise,
1410 * it must be held for either reading or writing and will not be released.
1411 *
1412 * In most cases, get_user_pages or get_user_pages_fast should be used
1413 * instead of __get_user_pages. __get_user_pages should be used only if
1414 * you need some special @gup_flags.
1415 */
1416 static long __get_user_pages(struct mm_struct *mm,
1417 unsigned long start, unsigned long nr_pages,
1418 unsigned int gup_flags, struct page **pages,
1419 int *locked)
1420 {
1421 long ret = 0, i = 0;
1422 struct vm_area_struct *vma = NULL;
1423 struct follow_page_context ctx = { NULL };
1424
1425 if (!nr_pages)
1426 return 0;
1427
1428 start = untagged_addr_remote(mm, start);
1429
1430 VM_BUG_ON(!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN)));
1431
1432 /* FOLL_GET and FOLL_PIN are mutually exclusive. */
> 1433 if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) ==
1434 (FOLL_PIN | FOLL_GET)))
> 1435 return ERR_PTR(-EINVAL);
1436
1437 do {
1438 struct page *page;
1439 unsigned int page_increm;
1440
1441 /* first iteration or cross vma bound */
1442 if (!vma || start >= vma->vm_end) {
1443 /*
1444 * MADV_POPULATE_(READ|WRITE) wants to handle VMA
1445 * lookups+error reporting differently.
1446 */
1447 if (gup_flags & FOLL_MADV_POPULATE) {
1448 vma = vma_lookup(mm, start);
1449 if (!vma) {
1450 ret = -ENOMEM;
1451 goto out;
1452 }
1453 if (check_vma_flags(vma, gup_flags)) {
1454 ret = -EINVAL;
1455 goto out;
1456 }
1457 goto retry;
1458 }
1459 vma = gup_vma_lookup(mm, start);
1460 if (!vma && in_gate_area(mm, start)) {
1461 ret = get_gate_page(mm, start & PAGE_MASK,
1462 gup_flags, &vma,
1463 pages ? &page : NULL);
1464 if (ret)
1465 goto out;
1466 ctx.page_mask = 0;
1467 goto next_page;
1468 }
1469
1470 if (!vma) {
1471 ret = -EFAULT;
1472 goto out;
1473 }
1474 ret = check_vma_flags(vma, gup_flags);
1475 if (ret)
1476 goto out;
1477 }
1478 retry:
1479 /*
1480 * If we have a pending SIGKILL, don't keep faulting pages and
1481 * potentially allocating memory.
1482 */
1483 if (fatal_signal_pending(current)) {
1484 ret = -EINTR;
1485 goto out;
1486 }
1487 cond_resched();
1488
1489 page = follow_page_mask(vma, start, gup_flags, &ctx);
1490 if (!page || PTR_ERR(page) == -EMLINK) {
1491 ret = faultin_page(vma, start, gup_flags,
1492 PTR_ERR(page) == -EMLINK, locked);
1493 switch (ret) {
1494 case 0:
1495 goto retry;
1496 case -EBUSY:
1497 case -EAGAIN:
1498 ret = 0;
1499 fallthrough;
1500 case -EFAULT:
1501 case -ENOMEM:
1502 case -EHWPOISON:
1503 goto out;
1504 }
1505 BUG();
1506 } else if (PTR_ERR(page) == -EEXIST) {
1507 /*
1508 * Proper page table entry exists, but no corresponding
1509 * struct page. If the caller expects **pages to be
1510 * filled in, bail out now, because that can't be done
1511 * for this page.
1512 */
1513 if (pages) {
1514 ret = PTR_ERR(page);
1515 goto out;
1516 }
1517 } else if (IS_ERR(page)) {
1518 ret = PTR_ERR(page);
1519 goto out;
1520 }
1521 next_page:
1522 page_increm = 1 + (~(start >> PAGE_SHIFT) & ctx.page_mask);
1523 if (page_increm > nr_pages)
1524 page_increm = nr_pages;
1525
1526 if (pages) {
1527 struct page *subpage;
1528 unsigned int j;
1529
1530 /*
1531 * This must be a large folio (and doesn't need to
1532 * be the whole folio; it can be part of it), do
1533 * the refcount work for all the subpages too.
1534 *
1535 * NOTE: here the page may not be the head page
1536 * e.g. when start addr is not thp-size aligned.
1537 * try_grab_folio() should have taken care of tail
1538 * pages.
1539 */
1540 if (page_increm > 1) {
1541 struct folio *folio = page_folio(page);
1542
1543 /*
1544 * Since we already hold refcount on the
1545 * large folio, this should never fail.
1546 */
1547 if (try_grab_folio(folio, page_increm - 1,
1548 gup_flags)) {
1549 /*
1550 * Release the 1st page ref if the
1551 * folio is problematic, fail hard.
1552 */
1553 gup_put_folio(folio, 1, gup_flags);
1554 ret = -EFAULT;
1555 goto out;
1556 }
1557 }
1558
1559 for (j = 0; j < page_increm; j++) {
1560 subpage = nth_page(page, j);
1561 pages[i + j] = subpage;
1562 flush_anon_page(vma, subpage, start + j * PAGE_SIZE);
1563 flush_dcache_page(subpage);
1564 }
1565 }
1566
1567 i += page_increm;
1568 start += page_increm * PAGE_SIZE;
1569 nr_pages -= page_increm;
1570 } while (nr_pages);
1571 out:
1572 if (ctx.pgmap)
1573 put_dev_pagemap(ctx.pgmap);
1574 return i ? i : ret;
1575 }
1576
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set in __get_user_pages() earlier
2025-03-30 13:49 ` kernel test robot
@ 2025-03-30 14:19 ` Baoquan He
0 siblings, 0 replies; 16+ messages in thread
From: Baoquan He @ 2025-03-30 14:19 UTC (permalink / raw)
To: kernel test robot; +Cc: akpm, oe-kbuild-all, linux-mm, linux-kernel
On 03/30/25 at 09:49pm, kernel test robot wrote:
> Hi Baoquan,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on akpm-mm/mm-everything]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Baoquan-He/mm-gup-fix-wrongly-calculated-returned-value-in-fault_in_safe_writeable/20250330-201949
> base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
> patch link: https://lore.kernel.org/r/20250330121718.175815-3-bhe%40redhat.com
> patch subject: [PATCH 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set in __get_user_pages() earlier
> config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20250330/202503302151.MdrisJhx-lkp@intel.com/config)
> compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250330/202503302151.MdrisJhx-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202503302151.MdrisJhx-lkp@intel.com/
Thanks for testing this patchset and reporting this error.
>
> All warnings (new ones prefixed by >>):
>
> In file included from arch/x86/include/asm/bug.h:110,
> from include/linux/bug.h:5,
> from include/linux/thread_info.h:13,
> from include/linux/spinlock.h:60,
> from mm/gup.c:5:
> mm/gup.c: In function '__get_user_pages':
> mm/gup.c:1433:27: error: 'flags' undeclared (first use in this function)
> 1433 | if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) ==
> | ^~~~~
It's a copy-and-paste error, below change can fix it. It's weird my
building didn't figure it out. I will re-run a building on the patchset,
then send v2 for patch 2.
diff --git a/mm/gup.c b/mm/gup.c
index 3d2c57f59b4d..1ae41c1b2649 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1430,7 +1430,7 @@ static long __get_user_pages(struct mm_struct *mm,
VM_BUG_ON(!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN)));
/* FOLL_GET and FOLL_PIN are mutually exclusive. */
- if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) ==
+ if (WARN_ON_ONCE((gup_flags & (FOLL_PIN | FOLL_GET)) ==
(FOLL_PIN | FOLL_GET)))
return ERR_PTR(-EINVAL);
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set in __get_user_pages() earlier
2025-03-30 12:17 ` [PATCH 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set in __get_user_pages() earlier Baoquan He
2025-03-30 13:49 ` kernel test robot
2025-03-30 14:09 ` kernel test robot
@ 2025-03-30 14:20 ` kernel test robot
2 siblings, 0 replies; 16+ messages in thread
From: kernel test robot @ 2025-03-30 14:20 UTC (permalink / raw)
To: Baoquan He, akpm; +Cc: oe-kbuild-all, linux-mm, linux-kernel, Baoquan He
Hi Baoquan,
kernel test robot noticed the following build errors:
[auto build test ERROR on akpm-mm/mm-everything]
url: https://github.com/intel-lab-lkp/linux/commits/Baoquan-He/mm-gup-fix-wrongly-calculated-returned-value-in-fault_in_safe_writeable/20250330-201949
base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link: https://lore.kernel.org/r/20250330121718.175815-3-bhe%40redhat.com
patch subject: [PATCH 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set in __get_user_pages() earlier
config: arc-randconfig-002-20250330 (https://download.01.org/0day-ci/archive/20250330/202503302101.y3r0RUgU-lkp@intel.com/config)
compiler: arc-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250330/202503302101.y3r0RUgU-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503302101.y3r0RUgU-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from include/linux/array_size.h:5,
from include/linux/kernel.h:16,
from mm/gup.c:2:
mm/gup.c: In function '__get_user_pages':
mm/gup.c:1433:27: error: 'flags' undeclared (first use in this function)
1433 | if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) ==
| ^~~~~
include/linux/compiler.h:57:52: note: in definition of macro '__trace_if_var'
57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
| ^~~~
mm/gup.c:1433:9: note: in expansion of macro 'if'
1433 | if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) ==
| ^~
include/asm-generic/bug.h:194:33: note: in expansion of macro 'WARN_ON'
194 | #define WARN_ON_ONCE(condition) WARN_ON(condition)
| ^~~~~~~
mm/gup.c:1433:13: note: in expansion of macro 'WARN_ON_ONCE'
1433 | if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) ==
| ^~~~~~~~~~~~
mm/gup.c:1433:27: note: each undeclared identifier is reported only once for each function it appears in
1433 | if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) ==
| ^~~~~
include/linux/compiler.h:57:52: note: in definition of macro '__trace_if_var'
57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
| ^~~~
mm/gup.c:1433:9: note: in expansion of macro 'if'
1433 | if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) ==
| ^~
include/asm-generic/bug.h:194:33: note: in expansion of macro 'WARN_ON'
194 | #define WARN_ON_ONCE(condition) WARN_ON(condition)
| ^~~~~~~
mm/gup.c:1433:13: note: in expansion of macro 'WARN_ON_ONCE'
1433 | if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) ==
| ^~~~~~~~~~~~
>> mm/gup.c:1435:24: error: returning 'void *' from a function with return type 'long int' makes integer from pointer without a cast [-Wint-conversion]
1435 | return ERR_PTR(-EINVAL);
| ^~~~~~~~~~~~~~~~
vim +1435 mm/gup.c
1361
1362 /**
1363 * __get_user_pages() - pin user pages in memory
1364 * @mm: mm_struct of target mm
1365 * @start: starting user address
1366 * @nr_pages: number of pages from start to pin
1367 * @gup_flags: flags modifying pin behaviour
1368 * @pages: array that receives pointers to the pages pinned.
1369 * Should be at least nr_pages long. Or NULL, if caller
1370 * only intends to ensure the pages are faulted in.
1371 * @locked: whether we're still with the mmap_lock held
1372 *
1373 * Returns either number of pages pinned (which may be less than the
1374 * number requested), or an error. Details about the return value:
1375 *
1376 * -- If nr_pages is 0, returns 0.
1377 * -- If nr_pages is >0, but no pages were pinned, returns -errno.
1378 * -- If nr_pages is >0, and some pages were pinned, returns the number of
1379 * pages pinned. Again, this may be less than nr_pages.
1380 * -- 0 return value is possible when the fault would need to be retried.
1381 *
1382 * The caller is responsible for releasing returned @pages, via put_page().
1383 *
1384 * Must be called with mmap_lock held. It may be released. See below.
1385 *
1386 * __get_user_pages walks a process's page tables and takes a reference to
1387 * each struct page that each user address corresponds to at a given
1388 * instant. That is, it takes the page that would be accessed if a user
1389 * thread accesses the given user virtual address at that instant.
1390 *
1391 * This does not guarantee that the page exists in the user mappings when
1392 * __get_user_pages returns, and there may even be a completely different
1393 * page there in some cases (eg. if mmapped pagecache has been invalidated
1394 * and subsequently re-faulted). However it does guarantee that the page
1395 * won't be freed completely. And mostly callers simply care that the page
1396 * contains data that was valid *at some point in time*. Typically, an IO
1397 * or similar operation cannot guarantee anything stronger anyway because
1398 * locks can't be held over the syscall boundary.
1399 *
1400 * If @gup_flags & FOLL_WRITE == 0, the page must not be written to. If
1401 * the page is written to, set_page_dirty (or set_page_dirty_lock, as
1402 * appropriate) must be called after the page is finished with, and
1403 * before put_page is called.
1404 *
1405 * If FOLL_UNLOCKABLE is set without FOLL_NOWAIT then the mmap_lock may
1406 * be released. If this happens *@locked will be set to 0 on return.
1407 *
1408 * A caller using such a combination of @gup_flags must therefore hold the
1409 * mmap_lock for reading only, and recognize when it's been released. Otherwise,
1410 * it must be held for either reading or writing and will not be released.
1411 *
1412 * In most cases, get_user_pages or get_user_pages_fast should be used
1413 * instead of __get_user_pages. __get_user_pages should be used only if
1414 * you need some special @gup_flags.
1415 */
1416 static long __get_user_pages(struct mm_struct *mm,
1417 unsigned long start, unsigned long nr_pages,
1418 unsigned int gup_flags, struct page **pages,
1419 int *locked)
1420 {
1421 long ret = 0, i = 0;
1422 struct vm_area_struct *vma = NULL;
1423 struct follow_page_context ctx = { NULL };
1424
1425 if (!nr_pages)
1426 return 0;
1427
1428 start = untagged_addr_remote(mm, start);
1429
1430 VM_BUG_ON(!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN)));
1431
1432 /* FOLL_GET and FOLL_PIN are mutually exclusive. */
1433 if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) ==
1434 (FOLL_PIN | FOLL_GET)))
> 1435 return ERR_PTR(-EINVAL);
1436
1437 do {
1438 struct page *page;
1439 unsigned int page_increm;
1440
1441 /* first iteration or cross vma bound */
1442 if (!vma || start >= vma->vm_end) {
1443 /*
1444 * MADV_POPULATE_(READ|WRITE) wants to handle VMA
1445 * lookups+error reporting differently.
1446 */
1447 if (gup_flags & FOLL_MADV_POPULATE) {
1448 vma = vma_lookup(mm, start);
1449 if (!vma) {
1450 ret = -ENOMEM;
1451 goto out;
1452 }
1453 if (check_vma_flags(vma, gup_flags)) {
1454 ret = -EINVAL;
1455 goto out;
1456 }
1457 goto retry;
1458 }
1459 vma = gup_vma_lookup(mm, start);
1460 if (!vma && in_gate_area(mm, start)) {
1461 ret = get_gate_page(mm, start & PAGE_MASK,
1462 gup_flags, &vma,
1463 pages ? &page : NULL);
1464 if (ret)
1465 goto out;
1466 ctx.page_mask = 0;
1467 goto next_page;
1468 }
1469
1470 if (!vma) {
1471 ret = -EFAULT;
1472 goto out;
1473 }
1474 ret = check_vma_flags(vma, gup_flags);
1475 if (ret)
1476 goto out;
1477 }
1478 retry:
1479 /*
1480 * If we have a pending SIGKILL, don't keep faulting pages and
1481 * potentially allocating memory.
1482 */
1483 if (fatal_signal_pending(current)) {
1484 ret = -EINTR;
1485 goto out;
1486 }
1487 cond_resched();
1488
1489 page = follow_page_mask(vma, start, gup_flags, &ctx);
1490 if (!page || PTR_ERR(page) == -EMLINK) {
1491 ret = faultin_page(vma, start, gup_flags,
1492 PTR_ERR(page) == -EMLINK, locked);
1493 switch (ret) {
1494 case 0:
1495 goto retry;
1496 case -EBUSY:
1497 case -EAGAIN:
1498 ret = 0;
1499 fallthrough;
1500 case -EFAULT:
1501 case -ENOMEM:
1502 case -EHWPOISON:
1503 goto out;
1504 }
1505 BUG();
1506 } else if (PTR_ERR(page) == -EEXIST) {
1507 /*
1508 * Proper page table entry exists, but no corresponding
1509 * struct page. If the caller expects **pages to be
1510 * filled in, bail out now, because that can't be done
1511 * for this page.
1512 */
1513 if (pages) {
1514 ret = PTR_ERR(page);
1515 goto out;
1516 }
1517 } else if (IS_ERR(page)) {
1518 ret = PTR_ERR(page);
1519 goto out;
1520 }
1521 next_page:
1522 page_increm = 1 + (~(start >> PAGE_SHIFT) & ctx.page_mask);
1523 if (page_increm > nr_pages)
1524 page_increm = nr_pages;
1525
1526 if (pages) {
1527 struct page *subpage;
1528 unsigned int j;
1529
1530 /*
1531 * This must be a large folio (and doesn't need to
1532 * be the whole folio; it can be part of it), do
1533 * the refcount work for all the subpages too.
1534 *
1535 * NOTE: here the page may not be the head page
1536 * e.g. when start addr is not thp-size aligned.
1537 * try_grab_folio() should have taken care of tail
1538 * pages.
1539 */
1540 if (page_increm > 1) {
1541 struct folio *folio = page_folio(page);
1542
1543 /*
1544 * Since we already hold refcount on the
1545 * large folio, this should never fail.
1546 */
1547 if (try_grab_folio(folio, page_increm - 1,
1548 gup_flags)) {
1549 /*
1550 * Release the 1st page ref if the
1551 * folio is problematic, fail hard.
1552 */
1553 gup_put_folio(folio, 1, gup_flags);
1554 ret = -EFAULT;
1555 goto out;
1556 }
1557 }
1558
1559 for (j = 0; j < page_increm; j++) {
1560 subpage = nth_page(page, j);
1561 pages[i + j] = subpage;
1562 flush_anon_page(vma, subpage, start + j * PAGE_SIZE);
1563 flush_dcache_page(subpage);
1564 }
1565 }
1566
1567 i += page_increm;
1568 start += page_increm * PAGE_SIZE;
1569 nr_pages -= page_increm;
1570 } while (nr_pages);
1571 out:
1572 if (ctx.pgmap)
1573 put_dev_pagemap(ctx.pgmap);
1574 return i ? i : ret;
1575 }
1576
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/7] mm/gup: fix wrongly calculated returned value in fault_in_safe_writeable()
2025-03-30 12:17 ` [PATCH 1/7] mm/gup: fix wrongly calculated returned value in fault_in_safe_writeable() Baoquan He
@ 2025-03-30 19:43 ` Zhu Yanjun
2025-03-30 22:48 ` Baoquan He
0 siblings, 1 reply; 16+ messages in thread
From: Zhu Yanjun @ 2025-03-30 19:43 UTC (permalink / raw)
To: Baoquan He, akpm; +Cc: linux-mm, linux-kernel
在 2025/3/30 14:17, Baoquan He 写道:
> Not like fault_in_writeable() or fault_in_writeable(), in
fault_in_readable()?
In the above, one of the 2 fault_in_writeable should be
fault_in_readable() ?
Zhu Yanjun
> fault_in_safe_writeable() local variable 'start' is increased page
> by page to loop till the whole address range is handled. However,
> it mistakenly calcalates the size of handled range with 'uaddr - start'.
>
> Fix it here.
>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
> mm/gup.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/gup.c b/mm/gup.c
> index 855ab860f88b..73777b1de679 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2207,8 +2207,8 @@ size_t fault_in_safe_writeable(const char __user *uaddr, size_t size)
> } while (start != end);
> mmap_read_unlock(mm);
>
> - if (size > (unsigned long)uaddr - start)
> - return size - ((unsigned long)uaddr - start);
> + if (size > start - (unsigned long)uaddr)
> + return size - (start - (unsigned long)uaddr);
> return 0;
> }
> EXPORT_SYMBOL(fault_in_safe_writeable);
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/7] mm/gup: fix wrongly calculated returned value in fault_in_safe_writeable()
2025-03-30 19:43 ` Zhu Yanjun
@ 2025-03-30 22:48 ` Baoquan He
0 siblings, 0 replies; 16+ messages in thread
From: Baoquan He @ 2025-03-30 22:48 UTC (permalink / raw)
To: Zhu Yanjun; +Cc: akpm, linux-mm, linux-kernel
On 03/30/25 at 09:43pm, Zhu Yanjun wrote:
> 在 2025/3/30 14:17, Baoquan He 写道:
> > Not like fault_in_writeable() or fault_in_writeable(), in
> fault_in_readable()?
>
> In the above, one of the 2 fault_in_writeable should be fault_in_readable()
> ?
You are right, I will fix it in v2. Thanks.
> > fault_in_safe_writeable() local variable 'start' is increased page
> > by page to loop till the whole address range is handled. However,
> > it mistakenly calcalates the size of handled range with 'uaddr - start'.
> >
> > Fix it here.
> >
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> > mm/gup.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/gup.c b/mm/gup.c
> > index 855ab860f88b..73777b1de679 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -2207,8 +2207,8 @@ size_t fault_in_safe_writeable(const char __user *uaddr, size_t size)
> > } while (start != end);
> > mmap_read_unlock(mm);
> > - if (size > (unsigned long)uaddr - start)
> > - return size - ((unsigned long)uaddr - start);
> > + if (size > start - (unsigned long)uaddr)
> > + return size - (start - (unsigned long)uaddr);
> > return 0;
> > }
> > EXPORT_SYMBOL(fault_in_safe_writeable);
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/7] mm/gup: Fix the outdated code comments above get_user_pages_unlocked()
2025-03-30 12:17 ` [PATCH 3/7] mm/gup: Fix the outdated code comments above get_user_pages_unlocked() Baoquan He
@ 2025-03-31 13:58 ` Oscar Salvador
2025-03-31 15:19 ` Baoquan He
0 siblings, 1 reply; 16+ messages in thread
From: Oscar Salvador @ 2025-03-31 13:58 UTC (permalink / raw)
To: Baoquan He; +Cc: akpm, linux-mm, linux-kernel
On Sun, Mar 30, 2025 at 08:17:13PM +0800, Baoquan He wrote:
> Since commit f0818f472d8d ("mm: gup: add get_user_pages_locked
> and get_user_pages_unlocked"), get_user_pages() doesn't need to have
> mmap_lock held anymore. It calls __get_user_pages_locked() which
> can acquire and drop the mmap_lock internaly.
Yes, __get_user_pages_locked() can aquire and drop the lock, but AFAICS
get_user_pages() always calls __get_user_pages_locked() with locked=1,
which means that is holding the lock, right?
> Hence remove the incorrect code comments now.
>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
> mm/gup.c | 16 +++-------------
> 1 file changed, 3 insertions(+), 13 deletions(-)
>
> diff --git a/mm/gup.c b/mm/gup.c
> index 8788105daee8..3345a065c2cb 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2702,19 +2702,9 @@ long get_user_pages(unsigned long start, unsigned long nr_pages,
> EXPORT_SYMBOL(get_user_pages);
>
> /*
> - * get_user_pages_unlocked() is suitable to replace the form:
> - *
> - * mmap_read_lock(mm);
> - * get_user_pages(mm, ..., pages, NULL);
> - * mmap_read_unlock(mm);
> - *
> - * with:
> - *
> - * get_user_pages_unlocked(mm, ..., pages);
> - *
> - * It is functionally equivalent to get_user_pages_fast so
> - * get_user_pages_fast should be used instead if specific gup_flags
> - * (e.g. FOLL_FORCE) are not required.
> + * get_user_pages_unlocked() is functionally equivalent to
> + * get_user_pages_fast so get_user_pages_fast should be used instead
> + * if specific gup_flags (e.g. FOLL_FORCE) are not required.
> */
> long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages,
> struct page **pages, unsigned int gup_flags)
> --
> 2.41.0
>
>
--
Oscar Salvador
SUSE Labs
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/7] mm/gup: Fix the outdated code comments above get_user_pages_unlocked()
2025-03-31 13:58 ` Oscar Salvador
@ 2025-03-31 15:19 ` Baoquan He
0 siblings, 0 replies; 16+ messages in thread
From: Baoquan He @ 2025-03-31 15:19 UTC (permalink / raw)
To: Oscar Salvador; +Cc: akpm, linux-mm, linux-kernel
On 03/31/25 at 03:58pm, Oscar Salvador wrote:
> On Sun, Mar 30, 2025 at 08:17:13PM +0800, Baoquan He wrote:
> > Since commit f0818f472d8d ("mm: gup: add get_user_pages_locked
> > and get_user_pages_unlocked"), get_user_pages() doesn't need to have
> > mmap_lock held anymore. It calls __get_user_pages_locked() which
> > can acquire and drop the mmap_lock internaly.
>
> Yes, __get_user_pages_locked() can aquire and drop the lock, but AFAICS
> get_user_pages() always calls __get_user_pages_locked() with locked=1,
> which means that is holding the lock, right?
Ah, You are right. Thanks for looking into this, Oscar. I incredibly
missed the local virable definition "int locked = 1;" line. I will drop
this patch, or wrap the code comment fix into other patch about the
obsolete reference with "get_user_pages(mm, ..., pages, NULL)".
>
> > Hence remove the incorrect code comments now.
> >
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> > mm/gup.c | 16 +++-------------
> > 1 file changed, 3 insertions(+), 13 deletions(-)
> >
> > diff --git a/mm/gup.c b/mm/gup.c
> > index 8788105daee8..3345a065c2cb 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -2702,19 +2702,9 @@ long get_user_pages(unsigned long start, unsigned long nr_pages,
> > EXPORT_SYMBOL(get_user_pages);
> >
> > /*
> > - * get_user_pages_unlocked() is suitable to replace the form:
> > - *
> > - * mmap_read_lock(mm);
> > - * get_user_pages(mm, ..., pages, NULL);
> > - * mmap_read_unlock(mm);
> > - *
> > - * with:
> > - *
> > - * get_user_pages_unlocked(mm, ..., pages);
> > - *
> > - * It is functionally equivalent to get_user_pages_fast so
> > - * get_user_pages_fast should be used instead if specific gup_flags
> > - * (e.g. FOLL_FORCE) are not required.
> > + * get_user_pages_unlocked() is functionally equivalent to
> > + * get_user_pages_fast so get_user_pages_fast should be used instead
> > + * if specific gup_flags (e.g. FOLL_FORCE) are not required.
> > */
> > long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages,
> > struct page **pages, unsigned int gup_flags)
> > --
> > 2.41.0
> >
> >
>
> --
> Oscar Salvador
> SUSE Labs
>
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2025-03-31 16:23 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-03-30 12:17 [PATCH 0/7] mm/gup: Minor fix, cleanup and improvements Baoquan He
2025-03-30 12:17 ` [PATCH 1/7] mm/gup: fix wrongly calculated returned value in fault_in_safe_writeable() Baoquan He
2025-03-30 19:43 ` Zhu Yanjun
2025-03-30 22:48 ` Baoquan He
2025-03-30 12:17 ` [PATCH 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set in __get_user_pages() earlier Baoquan He
2025-03-30 13:49 ` kernel test robot
2025-03-30 14:19 ` Baoquan He
2025-03-30 14:09 ` kernel test robot
2025-03-30 14:20 ` kernel test robot
2025-03-30 12:17 ` [PATCH 3/7] mm/gup: Fix the outdated code comments above get_user_pages_unlocked() Baoquan He
2025-03-31 13:58 ` Oscar Salvador
2025-03-31 15:19 ` Baoquan He
2025-03-30 12:17 ` [PATCH 4/7] mm/gup: remove gup_fast_pgd_leaf() and clean up the relevant codes Baoquan He
2025-03-30 12:17 ` [PATCH 5/7] x86/mm: remove pgd_leaf definition in arch Baoquan He
2025-03-30 12:17 ` [PATCH 6/7] x86/mm: remove p4d_leaf definition Baoquan He
2025-03-30 12:17 ` [PATCH 7/7] mm/pgtable: remove unneeded pgd_devmap() Baoquan He
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox