[PATCH v2 0/7] mm/gup: Minor fix, cleanup and improvements

[PATCH v2 0/7] mm/gup: Minor fix, cleanup and improvements

[Baoquan He]

These are made when I explore codes in mm/gup.c.

v1->v2:
- In patch 1, I carelessly copied the fault_in_readable() as
  fault_in_writeable(). Thanks to Yanjun for pointing it out.

- In patch 2, I copied the code in follow_page_pte() to
  __get_user_pages() directly w/o adjustment which is done but not
  merged to patch. That failed testing taken by lkp test robot, thanks
  for reporting. 

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

[PATCH v2 1/7] mm/gup: fix wrongly calculated returned value in fault_in_safe_writeable()

[Baoquan He]

Not like fault_in_readable() 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>
---
v1->v2:
- Fix a patch log typo caused by copy-and-paste error. Thanks
  to Yanjun.

 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

[PATCH v2 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set in __get_user_pages() earlier

[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>
---
v1->v2:
- Fix code bug caused by copy-and-paste error, this is reported by
  lkp test robot.

 mm/gup.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 73777b1de679..f9bce14ed3cd 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((gup_flags & (FOLL_PIN | FOLL_GET)) ==
+			 (FOLL_PIN | FOLL_GET)))
+		return -EINVAL;
+
 	do {
 		struct page *page;
 		unsigned int page_increm;
-- 
2.41.0

[PATCH v2 3/7] mm/gup: Fix the outdated code comments above get_user_pages_unlocked()

[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 f9bce14ed3cd..a15317cf6641 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

[PATCH v2 4/7] mm/gup: remove gup_fast_pgd_leaf() and clean up the relevant codes

[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 a15317cf6641..58cdc5605a4a 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

[PATCH v2 5/7] x86/mm: remove pgd_leaf definition in arch

[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>
Cc: x86@kernel.org
---
 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

[PATCH v2 6/7] x86/mm: remove p4d_leaf definition

[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>
Cc: x86@kernel.org
---
 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

[PATCH v2 7/7] mm/pgtable: remove unneeded pgd_devmap()

[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

Re: [PATCH v2 6/7] x86/mm: remove p4d_leaf definition

[Ingo Molnar]

* Baoquan He <bhe@redhat.com> wrote:

> 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.


> -#define p4d_leaf p4d_leaf
> -static inline bool p4d_leaf(p4d_t p4d)
> -{
> -	/* No 512 GiB pages yet */
> -	return 0;
> -}

This comment was also incorrect I believe:

1 PTE entry on x86-64 covers 4K virtual memory, 512 PTE entries make up 
a 4K pagetable page, and each level of paging adds another level of 512 
pagetable entries:

 - level 0:                 4K pages
 - level 1: 512x    4K =   2MB 'large' pages
 - level 2: 512x   2MB =   1GB 'huge' pages
 - level 3: 512x   1GB = 512GB 'PGD' pages
 - level 4: 512x 512GB = 256TB 'P4D' pages

So the above comment should have said '256 TB' pages, unless there's 
some naming weirdness I missed.

Thanks,

	Ingo

Re: [PATCH v2 6/7] x86/mm: remove p4d_leaf definition

[Baoquan He]

On 03/31/25 at 11:57am, Ingo Molnar wrote:
> 
> * Baoquan He <bhe@redhat.com> wrote:
> 
> > 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.

Thanks a lot for cleaning up the patch logs and rearranging x86 patches
into tip tree.

> 
> 
> > -#define p4d_leaf p4d_leaf
> > -static inline bool p4d_leaf(p4d_t p4d)
> > -{
> > -	/* No 512 GiB pages yet */
> > -	return 0;
> > -}
> 
> This comment was also incorrect I believe:
> 
> 1 PTE entry on x86-64 covers 4K virtual memory, 512 PTE entries make up 
> a 4K pagetable page, and each level of paging adds another level of 512 
> pagetable entries:
> 
>  - level 0:                 4K pages
>  - level 1: 512x    4K =   2MB 'large' pages
>  - level 2: 512x   2MB =   1GB 'huge' pages
>  - level 3: 512x   1GB = 512GB 'PGD' pages
>  - level 4: 512x 512GB = 256TB 'P4D' pages
> 
> So the above comment should have said '256 TB' pages, unless there's 
> some naming weirdness I missed.

Hmm, there could be misunderstanding here. In 5-level paging, PGD is the
highest level, P4D is the next level of PGD. You may have reversed their
order. So one P4D entry maps one PUD table which is 512 x 1GB. Then the
mentioned comment seems correct to me.

Re: [PATCH v2 6/7] x86/mm: remove p4d_leaf definition

[Ingo Molnar]

* Baoquan He <bhe@redhat.com> wrote:

> > So the above comment should have said '256 TB' pages, unless 
> > there's some naming weirdness I missed.
> 
> Hmm, there could be misunderstanding here. In 5-level paging, PGD is 
> the highest level, P4D is the next level of PGD. You may have 
> reversed their order.

Erm, yes indeed I flipped those two, so the correct table should be:

  - level 0:                 4K pages
  - level 1: 512x    4K =   2MB 'large' pages
  - level 2: 512x   2MB =   1GB 'huge' pages
  - level 3: 512x   1GB = 512GB 'P4D' pages
  - level 4: 512x 512GB = 256TB 'PGD' pages

I'm wondering whether 512GB pages will be called 'terapages'. ;-)

Thanks,

	Ingo

Re: [PATCH v2 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set in __get_user_pages() earlier

[David Hildenbrand]

On 31.03.25 10:13, Baoquan He wrote:
> 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>
> ---
> v1->v2:
> - Fix code bug caused by copy-and-paste error, this is reported by
>    lkp test robot.
> 
>   mm/gup.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 73777b1de679..f9bce14ed3cd 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((gup_flags & (FOLL_PIN | FOLL_GET)) ==
> +			 (FOLL_PIN | FOLL_GET)))
> +		return -EINVAL;
> +

We already have that check in is_valid_gup_args(), that catches all 
external users that could possibly mess this up.

So we can just convert that into a VM_WARN_ON_ONCE(), and while doing 
that, we should convert the VM_BUG_ON() above to a VM_WARN_ON_ONCE() as 
well.

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 1/7] mm/gup: fix wrongly calculated returned value in fault_in_safe_writeable()

[David Hildenbrand]

On 31.03.25 10:13, Baoquan He wrote:
> Not like fault_in_readable() 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.

Fixes: ? Do we know of user-visible effects?

> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
> v1->v2:
> - Fix a patch log typo caused by copy-and-paste error. Thanks
>    to Yanjun.
> 
>   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);

Can we instead just use the uaddr and start variables like in 
fault_in_readable?

That is, turn "start" into a const and adjust uaddr instead.

(maybe we should also handle the types of these variables similar to as 
in fault_in_readable)

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 3/7] mm/gup: Fix the outdated code comments above get_user_pages_unlocked()

[David Hildenbrand]

On 31.03.25 10:13, 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.

s/internaly/internally/

But your statement is wrong. get_user_pages() must be called with the 
mmap_lock held, because it sets "int locked = 1;" when calling 
__get_user_pages_locked().

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 4/7] mm/gup: remove gup_fast_pgd_leaf() and clean up the relevant codes

[David Hildenbrand]

On 31.03.25 10:13, Baoquan He wrote:
> 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.

I wonder if that was in place to handle (prepare for) very large hugetlb 
folios. Until a while ago, follow_page_mask() did not have to handle 
these hugetlb folios.

But I assume it never got used.

Acked-by: David Hildenbrand <david@redhat.com>

> 
> 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 a15317cf6641..58cdc5605a4a 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);
>   }
-- 
Cheers,

David / dhildenb

Re: [PATCH v2 7/7] mm/pgtable: remove unneeded pgd_devmap()

[David Hildenbrand]

On 31.03.25 10:13, Baoquan He wrote:
> There's no user of pgd_devmap() now, remove it from all ARCH-es
> and linux/pgtable.h.

Drop this patch, Alistair will remove all that devmap stuff soon.

https://lore.kernel.org/linux-mm/42a318bcbb65931958e52ce4b1334f3d012cbd6f.1736488799.git-series.apopple@nvidia.com/

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 5/7] x86/mm: remove pgd_leaf definition in arch

[David Hildenbrand]

On 31.03.25 10:13, Baoquan He wrote:
> 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>
> Cc: x86@kernel.org
> ---

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 3/7] mm/gup: Fix the outdated code comments above get_user_pages_unlocked()

[Oscar Salvador]

On Mon, Mar 31, 2025 at 04:13:23PM +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.
> 
> Hence remove the incorrect code comments now.

Uhm, I think this one should be dropped according to 

https://lore.kernel.org/linux-mm/20250330121718.175815-4-bhe@redhat.com/T/#m1aa161016ab98a6d85bcb657a729e01cb98763ec

?


-- 
Oscar Salvador
SUSE Labs

Re: [PATCH v2 1/7] mm/gup: fix wrongly calculated returned value in fault_in_safe_writeable()

[Oscar Salvador]

On Tue, Apr 01, 2025 at 10:10:03AM +0200, David Hildenbrand wrote:
> On 31.03.25 10:13, Baoquan He wrote:
> > --- 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);
> 
> Can we instead just use the uaddr and start variables like in
> fault_in_readable?
> 
> That is, turn "start" into a const and adjust uaddr instead.

Yes, I think that would be much cleaner.

Otherwise, this looks good to me. 

-- 
Oscar Salvador
SUSE Labs

Re: [PATCH v2 5/7] x86/mm: remove pgd_leaf definition in arch

[Oscar Salvador]

On Mon, Mar 31, 2025 at 04:13:25PM +0800, Baoquan He wrote:
> 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>
> Cc: x86@kernel.org

I have been carrying a sort of this patch in my tree as well for quite
some time now, and I think that Christophe also sent it some time ago,
so glad someone finally pushed it

https://patchwork.kernel.org/project/linux-mm/patch/20240704043132.28501-2-osalvador@suse.de/

Reviewed-by: Oscar Salvador <osalvador@suse.de>


-- 
Oscar Salvador
SUSE Labs

Re: [PATCH v2 6/7] x86/mm: remove p4d_leaf definition

[Oscar Salvador]

On Mon, Mar 31, 2025 at 04:13:26PM +0800, Baoquan He wrote:
> 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>
> Cc: x86@kernel.org

Reviewed-by: Oscar Salvador <osalvador@suse.de>


-- 
Oscar Salvador
SUSE Labs

Re: [PATCH v2 4/7] mm/gup: remove gup_fast_pgd_leaf() and clean up the relevant codes

[Oscar Salvador]

On Mon, Mar 31, 2025 at 04:13:24PM +0800, Baoquan He wrote:
> 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>

Reviewed-by: Oscar Salvador <osalvador@suse.de>

 

-- 
Oscar Salvador
SUSE Labs

Re: [PATCH v2 1/7] mm/gup: fix wrongly calculated returned value in fault_in_safe_writeable()

[Baoquan He]

On 04/01/25 at 10:10am, David Hildenbrand wrote:
> On 31.03.25 10:13, Baoquan He wrote:
> > Not like fault_in_readable() 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.
> 
> Fixes: ? Do we know of user-visible effects?

I believe it should impact, while I didn't hear of any complaint.
Yeah, it's worth to have one Fixes.

> 
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> > v1->v2:
> > - Fix a patch log typo caused by copy-and-paste error. Thanks
> >    to Yanjun.
> > 
> >   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);
> 
> Can we instead just use the uaddr and start variables like in
> fault_in_readable?
> 
> That is, turn "start" into a const and adjust uaddr instead.

Sounds good to me, that makes these similar blocks own consistent code
style. Will change in v3. Thanks for reviewing.

> 
> (maybe we should also handle the types of these variables similar to as in
> fault_in_readable)
> 
> -- 
> Cheers,
> 
> David / dhildenb
> 

Re: [PATCH v2 1/7] mm/gup: fix wrongly calculated returned value in fault_in_safe_writeable()

[Baoquan He]

On 04/01/25 at 04:00pm, Oscar Salvador wrote:
> On Tue, Apr 01, 2025 at 10:10:03AM +0200, David Hildenbrand wrote:
> > On 31.03.25 10:13, Baoquan He wrote:
> > > --- 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);
> > 
> > Can we instead just use the uaddr and start variables like in
> > fault_in_readable?
> > 
> > That is, turn "start" into a const and adjust uaddr instead.
> 
> Yes, I think that would be much cleaner.
> 
> Otherwise, this looks good to me. 

Will change in v3 as both of you suggested, thanks for reviewing this v2
series.

Re: [PATCH v2 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set in __get_user_pages() earlier

[Baoquan He]

On 04/01/25 at 10:02am, David Hildenbrand wrote:
> On 31.03.25 10:13, Baoquan He wrote:
> > 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>
> > ---
> > v1->v2:
> > - Fix code bug caused by copy-and-paste error, this is reported by
> >    lkp test robot.
> > 
> >   mm/gup.c | 10 +++++-----
> >   1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/mm/gup.c b/mm/gup.c
> > index 73777b1de679..f9bce14ed3cd 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((gup_flags & (FOLL_PIN | FOLL_GET)) ==
> > +			 (FOLL_PIN | FOLL_GET)))
> > +		return -EINVAL;
> > +
> 
> We already have that check in is_valid_gup_args(), that catches all external
> users that could possibly mess this up.

Right.

> 
> So we can just convert that into a VM_WARN_ON_ONCE(), and while doing that,
> we should convert the VM_BUG_ON() above to a VM_WARN_ON_ONCE() as well.

Sounds great to me, will put below change into this patch of v3 as suggested.
Thanks.

diff --git a/mm/gup.c b/mm/gup.c
index 9e4ed09c578b..d551da9549b1 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1427,10 +1427,10 @@ static long __get_user_pages(struct mm_struct *mm,
 
 	start = untagged_addr_remote(mm, start);
 
-	VM_BUG_ON(!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN)));
+	VM_WARN_ON_ONCE(!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN)));
 
 	/* FOLL_GET and FOLL_PIN are mutually exclusive. */
-	if (WARN_ON_ONCE((gup_flags & (FOLL_PIN | FOLL_GET)) ==
+	if (VM_WARN_ON_ONCE((gup_flags & (FOLL_PIN | FOLL_GET)) ==
 			 (FOLL_PIN | FOLL_GET)))
 		return -EINVAL;

Re: [PATCH v2 3/7] mm/gup: Fix the outdated code comments above get_user_pages_unlocked()

[Baoquan He]

On 04/01/25 at 10:14am, David Hildenbrand wrote:
> On 31.03.25 10:13, 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.
> 
> s/internaly/internally/
> 
> But your statement is wrong. get_user_pages() must be called with the
> mmap_lock held, because it sets "int locked = 1;" when calling
> __get_user_pages_locked().

You are right. I missed that line. Oscar pointed that out in v1 thread.
I will drop this patch. Thanks.

Re: [PATCH v2 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set in __get_user_pages() earlier

[David Hildenbrand]

On 01.04.25 16:34, Baoquan He wrote:
> On 04/01/25 at 10:02am, David Hildenbrand wrote:
>> On 31.03.25 10:13, Baoquan He wrote:
>>> 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>
>>> ---
>>> v1->v2:
>>> - Fix code bug caused by copy-and-paste error, this is reported by
>>>     lkp test robot.
>>>
>>>    mm/gup.c | 10 +++++-----
>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/mm/gup.c b/mm/gup.c
>>> index 73777b1de679..f9bce14ed3cd 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((gup_flags & (FOLL_PIN | FOLL_GET)) ==
>>> +			 (FOLL_PIN | FOLL_GET)))
>>> +		return -EINVAL;
>>> +
>>
>> We already have that check in is_valid_gup_args(), that catches all external
>> users that could possibly mess this up.
> 
> Right.
> 
>>
>> So we can just convert that into a VM_WARN_ON_ONCE(), and while doing that,
>> we should convert the VM_BUG_ON() above to a VM_WARN_ON_ONCE() as well.
> 
> Sounds great to me, will put below change into this patch of v3 as suggested.
> Thanks.
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 9e4ed09c578b..d551da9549b1 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1427,10 +1427,10 @@ static long __get_user_pages(struct mm_struct *mm,
>   
>   	start = untagged_addr_remote(mm, start);
>   
> -	VM_BUG_ON(!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN)));
> +	VM_WARN_ON_ONCE(!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN)));
>   
>   	/* FOLL_GET and FOLL_PIN are mutually exclusive. */
> -	if (WARN_ON_ONCE((gup_flags & (FOLL_PIN | FOLL_GET)) ==
> +	if (VM_WARN_ON_ONCE((gup_flags & (FOLL_PIN | FOLL_GET)) ==
>   			 (FOLL_PIN | FOLL_GET)))

That won't work (or shouldn't work :) ). Just drop the handling, it's a 
sanity check we don't expect to ever trigger, just like the old 
VM_BUG_ON above.

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 3/7] mm/gup: Fix the outdated code comments above get_user_pages_unlocked()

[Baoquan He]

On 04/01/25 at 03:51pm, Oscar Salvador wrote:
> On Mon, Mar 31, 2025 at 04:13:23PM +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.
> > 
> > Hence remove the incorrect code comments now.
> 
> Uhm, I think this one should be dropped according to 

Yeah, this v2 series was sent earlier than your comment in v1. Sorry
about the mess.

> 
> https://lore.kernel.org/linux-mm/20250330121718.175815-4-bhe@redhat.com/T/#m1aa161016ab98a6d85bcb657a729e01cb98763ec
> 
> ?
> 
> 
> -- 
> Oscar Salvador
> SUSE Labs
> 

Re: [PATCH v2 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set in __get_user_pages() earlier

[Baoquan He]

On 04/01/25 at 04:37pm, David Hildenbrand wrote:
> On 01.04.25 16:34, Baoquan He wrote:
> > On 04/01/25 at 10:02am, David Hildenbrand wrote:
> > > On 31.03.25 10:13, Baoquan He wrote:
...snip...
> > > We already have that check in is_valid_gup_args(), that catches all external
> > > users that could possibly mess this up.
> > 
> > Right.
> > 
> > > 
> > > So we can just convert that into a VM_WARN_ON_ONCE(), and while doing that,
> > > we should convert the VM_BUG_ON() above to a VM_WARN_ON_ONCE() as well.
> > 
> > Sounds great to me, will put below change into this patch of v3 as suggested.
> > Thanks.
> > 
> > diff --git a/mm/gup.c b/mm/gup.c
> > index 9e4ed09c578b..d551da9549b1 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -1427,10 +1427,10 @@ static long __get_user_pages(struct mm_struct *mm,
> >   	start = untagged_addr_remote(mm, start);
> > -	VM_BUG_ON(!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN)));
> > +	VM_WARN_ON_ONCE(!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN)));
> >   	/* FOLL_GET and FOLL_PIN are mutually exclusive. */
> > -	if (WARN_ON_ONCE((gup_flags & (FOLL_PIN | FOLL_GET)) ==
> > +	if (VM_WARN_ON_ONCE((gup_flags & (FOLL_PIN | FOLL_GET)) ==
> >   			 (FOLL_PIN | FOLL_GET)))
> 
> That won't work (or shouldn't work :) ). Just drop the handling, it's a
> sanity check we don't expect to ever trigger, just like the old VM_BUG_ON
> above.

Ah, I misunderstood it. Will add below change into this patch of v3.

diff --git a/mm/gup.c b/mm/gup.c
index 9e4ed09c578b..a8a10a8ebf14 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1427,12 +1427,9 @@ static long __get_user_pages(struct mm_struct *mm,
 
 	start = untagged_addr_remote(mm, start);
 
-	VM_BUG_ON(!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN)));
-
-	/* FOLL_GET and FOLL_PIN are mutually exclusive. */
-	if (WARN_ON_ONCE((gup_flags & (FOLL_PIN | FOLL_GET)) ==
-			 (FOLL_PIN | FOLL_GET)))
-		return -EINVAL;
+	VM_WARN_ON_ONCE(!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN)));
+	VM_WARN_ON_ONCE((gup_flags & (FOLL_PIN | FOLL_GET)) ==
+			(FOLL_PIN | FOLL_GET));
 
 	do {
 		struct page *page;

Re: [PATCH v2 7/7] mm/pgtable: remove unneeded pgd_devmap()

[Baoquan He]

On 04/01/25 at 10:21am, David Hildenbrand wrote:
> On 31.03.25 10:13, Baoquan He wrote:
> > There's no user of pgd_devmap() now, remove it from all ARCH-es
> > and linux/pgtable.h.
> 
> Drop this patch, Alistair will remove all that devmap stuff soon.
> 
> https://lore.kernel.org/linux-mm/42a318bcbb65931958e52ce4b1334f3d012cbd6f.1736488799.git-series.apopple@nvidia.com/

Thanks for telling, will drop it.

> 
> -- 
> Cheers,
> 
> David / dhildenb
> 

Re: [PATCH v2 5/7] x86/mm: remove pgd_leaf definition in arch

[Baoquan He]

On 04/01/25 at 04:03pm, Oscar Salvador wrote:
> On Mon, Mar 31, 2025 at 04:13:25PM +0800, Baoquan He wrote:
> > 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>
> > Cc: x86@kernel.org
> 
> I have been carrying a sort of this patch in my tree as well for quite
> some time now, and I think that Christophe also sent it some time ago,
> so glad someone finally pushed it
> 
> https://patchwork.kernel.org/project/linux-mm/patch/20240704043132.28501-2-osalvador@suse.de/

Oops, I even commented in Christophe's patch thread to ask why the
patchset hasn't been merged yet. Later when I focused on mm/gup.c code
reading, I forgot that posting completely. Hope you don't mind I grab
you and Christophe's credit since Ingo has picked it into x86/tip tree.

[PATCH v3 1/5] arch/x86: Drop own definition of pgd,p4d_leaf

> 
> Reviewed-by: Oscar Salvador <osalvador@suse.de>
> 
> 
> -- 
> Oscar Salvador
> SUSE Labs
> 

Re: [PATCH v2 6/7] x86/mm: remove p4d_leaf definition

[Baoquan He]

On 04/01/25 at 09:20am, Ingo Molnar wrote:
> 
> * Baoquan He <bhe@redhat.com> wrote:
> 
> > > So the above comment should have said '256 TB' pages, unless 
> > > there's some naming weirdness I missed.
> > 
> > Hmm, there could be misunderstanding here. In 5-level paging, PGD is 
> > the highest level, P4D is the next level of PGD. You may have 
> > reversed their order.
> 
> Erm, yes indeed I flipped those two, so the correct table should be:
> 
>   - level 0:                 4K pages
>   - level 1: 512x    4K =   2MB 'large' pages
>   - level 2: 512x   2MB =   1GB 'huge' pages
>   - level 3: 512x   1GB = 512GB 'P4D' pages
>   - level 4: 512x 512GB = 256TB 'PGD' pages
> 
> I'm wondering whether 512GB pages will be called 'terapages'. ;-)

Forgot replying to this one.

With my understanding, MM usually don't name large page of different
size a specific name, just call them hugepage + size, e.g 2M huge page,
or 1G huge page. Sometime when we build mapping for them, we will call
them PMD hugepage or PUD hugepage.

I remember in x86 ARCH direct mapping is built with invocation of
init_mem_mapping(), and the mapping to 2M or 1G directly is called
large page mapping or PMD/PUD huge page mapping. They are similar.

Hope other MM people can correct me if I am wrong.