[PATCH v4 0/4] mm/gup: Minor fix, cleanup and improvements

[PATCH v4 0/4] mm/gup: Minor fix, cleanup and improvements

[Baoquan He]

These were made from code inspection in mm/gup.c.

v3->v4:
========
- Make patch 1 to fix the code bug in fault_in_safe_writeable();
- Add patch 4 to do the code optimization to make fault_in_xxx()
  functions own the consistent code style;
- Add reviewing tags from David and Oscar for patch 2;

v2->v3:
=========
- In patch 1, change to use local variable 'start' as a loop cursor
  in fault_in_readable() and fault_in_writeable() so that they have the
  consistent code style with fault_in_safe_writeable(). Doing this can
  avoid ugly long line caused by kinds of type cast (const char __user *).
  Thanks to David who suggested two ways including this one, I found this
  is better when changing code.

- In patch2, changes to add VM_WARN_ON_ONCE() for both below chekcing
  because all external users of __get_user_pages() have done the
  checking in is_valid_gup_args() before calling __get_user_pages().
  Just adding these VM_WARN_ON_ONCE() in case, e.g internal setting
  wrong flags in kernel code. Thanks to David for suggesting this.

   - (gup_flags & (FOLL_PIN | FOLL_GET)) == (FOLL_PIN | FOLL_GET))
   - !!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN))

- Drop
      - the old patch 3 of v2 because it's from misunderstanding;
      - the old patch 4, 5 of v2 because they have been merged into x86
        tip tree by Ingo;
      - the old patch 7 of v2 because it has been coverred in another
        thread. Thanks to David for telling the thread link.

- In patch 3, add reviewing tags from Oscar and David.

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 (4):
  mm/gup: fix wrongly calculated returned value in
    fault_in_safe_writeable()
  mm/gup: remove unneeded checking in follow_page_pte()
  mm/gup: remove gup_fast_pgd_leaf() and clean up the relevant codes
  mm/gup: clean up codes in fault_in_xxx() functions

 mm/gup.c | 125 +++++++++++++++----------------------------------------
 1 file changed, 34 insertions(+), 91 deletions(-)

-- 
2.41.0

[PATCH v4 1/4] 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>
Fixes: fe673d3f5bf1 ("mm: gup: make fault_in_safe_writeable() use fixup_user_fault()")
---
 mm/gup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 92351e2fa876..84461d384ae2 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

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

[Oscar Salvador]

On Thu, Apr 10, 2025 at 11:57:14AM +0800, 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.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Fixes: fe673d3f5bf1 ("mm: gup: make fault_in_safe_writeable() use fixup_user_fault()")

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

> ---
>  mm/gup.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 92351e2fa876..84461d384ae2 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
> 

-- 
Oscar Salvador
SUSE Labs

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

[Andrew Morton]

On Thu, 10 Apr 2025 11:57:14 +0800 Baoquan He <bhe@redhat.com> 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'.

What are the userspace-visible runtime effects of this change?

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

[Baoquan He]

On 04/10/25 at 08:43pm, Andrew Morton wrote:
> On Thu, 10 Apr 2025 11:57:14 +0800 Baoquan He <bhe@redhat.com> 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'.
> 
> What are the userspace-visible runtime effects of this change?

I see it mainly affect gfs2_file_direct_read(). Not sure if GFS2 people
can sense any exceptional behaviour caused by this code bug.

> 

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

[Andreas Gruenbacher]

On Fri, Apr 11, 2025 at 7:32 AM Baoquan He <bhe@redhat.com> wrote:
> On 04/10/25 at 08:43pm, Andrew Morton wrote:
> > On Thu, 10 Apr 2025 11:57:14 +0800 Baoquan He <bhe@redhat.com> 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'.
> >
> > What are the userspace-visible runtime effects of this change?
>
> I see it mainly affect gfs2_file_direct_read(). Not sure if GFS2 people
> can sense any exceptional behaviour caused by this code bug.

Thanks for the heads up.

In gfs2, fault_in_iov_iter_writeable() is used in
gfs2_file_direct_read() and gfs2_file_read_iter(), so this potentially
affects buffered as well as direct reads. This bug could cause those
gfs2 functions to spin in a loop.

Can this fix please be sent to Linus for inclusion into 6.15?

Thanks,
Andreas

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

[Andrew Morton]

On Fri, 11 Apr 2025 17:07:32 +0200 Andreas Gruenbacher <agruenba@redhat.com> wrote:

> Can this fix please be sent to Linus for inclusion into 6.15?

Sure, I made the adjustments.

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

[David Hildenbrand]

On 10.04.25 05:57, 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.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Fixes: fe673d3f5bf1 ("mm: gup: make fault_in_safe_writeable() use fixup_user_fault()")
> ---
>   mm/gup.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 92351e2fa876..84461d384ae2 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);


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

-- 
Cheers,

David / dhildenb

[PATCH v4 2/4] mm/gup: remove unneeded checking in follow_page_pte()

[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 set. However, it's not supported both GUP_GET and GUP_PIN are set.
Even though this check need be done, it should be done earlier, but not
doing it till entering into follow_page_pte() and failed.

Furthermore, this checking has been done in is_valid_gup_args() and all
external users of __get_user_pages() will call is_valid_gup_args() to
catch the illegal setting. We don't need to worry about internal users
of __get_user_pages() because the gup_flags are set by MM code correctly.

Here remove the checking in follow_page_pte(), and add VM_WARN_ON_ONCE()
to catch the possible exceptional setting just in case.

And also change the VM_BUG_ON to VM_WARN_ON_ONCE() for checking
(!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN))) because the checking
has been done in is_valid_gup_args() for external users of
__get_user_pages().

Signed-off-by: Baoquan He <bhe@redhat.com>
Reviewed-by: Oscar Salvador <osalvador@suse.de>
Acked-by: David Hildenbrand <david@redhat.com>
---
 mm/gup.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 84461d384ae2..eb668da933e1 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -844,11 +844,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);
@@ -1432,7 +1427,11 @@ 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. */
+	VM_WARN_ON_ONCE((gup_flags & (FOLL_PIN | FOLL_GET)) ==
+			(FOLL_PIN | FOLL_GET));
 
 	do {
 		struct page *page;
-- 
2.41.0

[PATCH v4 3/4] 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 only
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>
Acked-by: David Hildenbrand <david@redhat.com>
---
 mm/gup.c | 49 +++----------------------------------------------
 1 file changed, 3 insertions(+), 46 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index eb668da933e1..77a5bc622567 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -3172,46 +3172,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)
@@ -3306,12 +3266,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 v4 4/4] mm/gup: clean up codes in fault_in_xxx() functions

[Baoquan He]

The code style in fault_in_readable() and fault_in_writable() is a
little inconsistent with fault_in_safe_writeable(). In fault_in_readable()
and fault_in_writable(), it uses 'uaddr' passed in as loop cursor. While
in fault_in_safe_writeable(), local variable 'start' is used as loop
cursor. This may mislead people when reading code or making change in
these codes.

Here define explicit loop cursor and use for loop to simplify codes in
these three functions. These cleanup can make them be consistent in
code style and improve readability.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/gup.c | 65 +++++++++++++++++++++++---------------------------------
 1 file changed, 26 insertions(+), 39 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 77a5bc622567..a76bd7e90a71 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2113,28 +2113,24 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
  */
 size_t fault_in_writeable(char __user *uaddr, size_t size)
 {
-	char __user *start = uaddr, *end;
+	const unsigned long start = (unsigned long)uaddr;
+	const unsigned long end = start + size;
+	unsigned long cur = start;
 
 	if (unlikely(size == 0))
 		return 0;
+
 	if (!user_write_access_begin(uaddr, size))
 		return size;
-	if (!PAGE_ALIGNED(uaddr)) {
-		unsafe_put_user(0, uaddr, out);
-		uaddr = (char __user *)PAGE_ALIGN((unsigned long)uaddr);
-	}
-	end = (char __user *)PAGE_ALIGN((unsigned long)start + size);
-	if (unlikely(end < start))
-		end = NULL;
-	while (uaddr != end) {
-		unsafe_put_user(0, uaddr, out);
-		uaddr += PAGE_SIZE;
-	}
+
+	/* Stop once we overflow to 0. */
+	for (; cur && cur < end; cur = PAGE_ALIGN_DOWN(cur + PAGE_SIZE))
+		unsafe_put_user(0, (char __user *)cur, out);
 
 out:
 	user_write_access_end();
-	if (size > uaddr - start)
-		return size - (uaddr - start);
+	if (size > cur - start)
+		return size - (cur - start);
 	return 0;
 }
 EXPORT_SYMBOL(fault_in_writeable);
@@ -2188,26 +2184,24 @@ EXPORT_SYMBOL(fault_in_subpage_writeable);
  */
 size_t fault_in_safe_writeable(const char __user *uaddr, size_t size)
 {
-	unsigned long start = (unsigned long)uaddr, end;
+	const unsigned long start = (unsigned long)uaddr;
+	const unsigned long end = start + size;
+	unsigned long cur = start;
 	struct mm_struct *mm = current->mm;
 	bool unlocked = false;
 
 	if (unlikely(size == 0))
 		return 0;
-	end = PAGE_ALIGN(start + size);
-	if (end < start)
-		end = 0;
 
 	mmap_read_lock(mm);
-	do {
-		if (fixup_user_fault(mm, start, FAULT_FLAG_WRITE, &unlocked))
+	/* Stop once we overflow to 0. */
+	for (; cur && cur < end; cur = PAGE_ALIGN_DOWN(cur + PAGE_SIZE))
+		if (fixup_user_fault(mm, cur, FAULT_FLAG_WRITE, &unlocked))
 			break;
-		start = (start + PAGE_SIZE) & PAGE_MASK;
-	} while (start != end);
 	mmap_read_unlock(mm);
 
-	if (size > start - (unsigned long)uaddr)
-		return size - (start - (unsigned long)uaddr);
+	if (size > cur - start)
+		return size - (cur - start);
 	return 0;
 }
 EXPORT_SYMBOL(fault_in_safe_writeable);
@@ -2222,30 +2216,23 @@ EXPORT_SYMBOL(fault_in_safe_writeable);
  */
 size_t fault_in_readable(const char __user *uaddr, size_t size)
 {
-	const char __user *start = uaddr, *end;
+	const unsigned long start = (unsigned long)uaddr;
+	const unsigned long end = start + size;
+	unsigned long cur = start;
 	volatile char c;
 
 	if (unlikely(size == 0))
 		return 0;
 	if (!user_read_access_begin(uaddr, size))
 		return size;
-	if (!PAGE_ALIGNED(uaddr)) {
-		unsafe_get_user(c, uaddr, out);
-		uaddr = (const char __user *)PAGE_ALIGN((unsigned long)uaddr);
-	}
-	end = (const char __user *)PAGE_ALIGN((unsigned long)start + size);
-	if (unlikely(end < start))
-		end = NULL;
-	while (uaddr != end) {
-		unsafe_get_user(c, uaddr, out);
-		uaddr += PAGE_SIZE;
-	}
-
+	/* Stop once we overflow to 0. */
+	for (; cur && cur < end; cur = PAGE_ALIGN_DOWN(cur + PAGE_SIZE))
+		unsafe_get_user(c, (const char __user *)cur, out);
 out:
 	user_read_access_end();
 	(void)c;
-	if (size > uaddr - start)
-		return size - (uaddr - start);
+	if (size > cur - start)
+		return size - (cur - start);
 	return 0;
 }
 EXPORT_SYMBOL(fault_in_readable);
-- 
2.41.0

Re: [PATCH v4 4/4] mm/gup: clean up codes in fault_in_xxx() functions

[David Hildenbrand]

On 10.04.25 05:57, Baoquan He wrote:
> The code style in fault_in_readable() and fault_in_writable() is a
> little inconsistent with fault_in_safe_writeable(). In fault_in_readable()
> and fault_in_writable(), it uses 'uaddr' passed in as loop cursor. While
> in fault_in_safe_writeable(), local variable 'start' is used as loop
> cursor. This may mislead people when reading code or making change in
> these codes.
> 
> Here define explicit loop cursor and use for loop to simplify codes in
> these three functions. These cleanup can make them be consistent in
> code style and improve readability.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>   mm/gup.c | 65 +++++++++++++++++++++++---------------------------------
>   1 file changed, 26 insertions(+), 39 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 77a5bc622567..a76bd7e90a71 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2113,28 +2113,24 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
>    */
>   size_t fault_in_writeable(char __user *uaddr, size_t size)
>   {
> -	char __user *start = uaddr, *end;
> +	const unsigned long start = (unsigned long)uaddr;
> +	const unsigned long end = start + size;
> +	unsigned long cur = start;

I would initialize cur in the for loop header, makes the loop easier to 
read.

>   
>   	if (unlikely(size == 0))
>   		return 0;
> +

Would not add that line to keep it like fault_in_readable() below.

>   	if (!user_write_access_begin(uaddr, size))
>   		return size;
> -	if (!PAGE_ALIGNED(uaddr)) {
> -		unsafe_put_user(0, uaddr, out);
> -		uaddr = (char __user *)PAGE_ALIGN((unsigned long)uaddr);
> -	}
> -	end = (char __user *)PAGE_ALIGN((unsigned long)start + size);
> -	if (unlikely(end < start))
> -		end = NULL;
> -	while (uaddr != end) {
> -		unsafe_put_user(0, uaddr, out);
> -		uaddr += PAGE_SIZE;
> -	}
> +
> +	/* Stop once we overflow to 0. */
> +	for (; cur && cur < end; cur = PAGE_ALIGN_DOWN(cur + PAGE_SIZE))
> +		unsafe_put_user(0, (char __user *)cur, out);
>   

Staring at fault_in_safe_writeable(), we could also do

/* Stop once we overflow to 0. */
end = PAGE_ALIGN(end)
if (start < end)
	end = 0;

for (cur = start; cur != end; cur = PAGE_ALIGN_DOWN(cur + PAGE_SIZE))
	unsafe_put_user(0, (char __user *)cur, out);

Essentially, removing the "cur" check from the loop condition. Not sure 
if that is better.

In any case, if all functions later look similar and clearer it's a big win.

-- 
Cheers,

David / dhildenb

Re: [PATCH v4 4/4] mm/gup: clean up codes in fault_in_xxx() functions

[Baoquan He]

On 04/11/25 at 10:54am, David Hildenbrand wrote:
> On 10.04.25 05:57, Baoquan He wrote:
> > The code style in fault_in_readable() and fault_in_writable() is a
> > little inconsistent with fault_in_safe_writeable(). In fault_in_readable()
> > and fault_in_writable(), it uses 'uaddr' passed in as loop cursor. While
> > in fault_in_safe_writeable(), local variable 'start' is used as loop
> > cursor. This may mislead people when reading code or making change in
> > these codes.
> > 
> > Here define explicit loop cursor and use for loop to simplify codes in
> > these three functions. These cleanup can make them be consistent in
> > code style and improve readability.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> >   mm/gup.c | 65 +++++++++++++++++++++++---------------------------------
> >   1 file changed, 26 insertions(+), 39 deletions(-)
> > 
> > diff --git a/mm/gup.c b/mm/gup.c
> > index 77a5bc622567..a76bd7e90a71 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -2113,28 +2113,24 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
> >    */
> >   size_t fault_in_writeable(char __user *uaddr, size_t size)
> >   {
> > -	char __user *start = uaddr, *end;
> > +	const unsigned long start = (unsigned long)uaddr;
> > +	const unsigned long end = start + size;
> > +	unsigned long cur = start;
> 
> I would initialize cur in the for loop header, makes the loop easier to
> read.

Both is fine to me. It's to satisfy checkpatch.sh which complains about
exceeding 80 char in the line.

> 
> >   	if (unlikely(size == 0))
> >   		return 0;
> > +
> 
> Would not add that line to keep it like fault_in_readable() below.

Will remove it.

> 
> >   	if (!user_write_access_begin(uaddr, size))
> >   		return size;
> > -	if (!PAGE_ALIGNED(uaddr)) {
> > -		unsafe_put_user(0, uaddr, out);
> > -		uaddr = (char __user *)PAGE_ALIGN((unsigned long)uaddr);
> > -	}
> > -	end = (char __user *)PAGE_ALIGN((unsigned long)start + size);
> > -	if (unlikely(end < start))
> > -		end = NULL;
> > -	while (uaddr != end) {
> > -		unsafe_put_user(0, uaddr, out);
> > -		uaddr += PAGE_SIZE;
> > -	}
> > +
> > +	/* Stop once we overflow to 0. */
> > +	for (; cur && cur < end; cur = PAGE_ALIGN_DOWN(cur + PAGE_SIZE))
> > +		unsafe_put_user(0, (char __user *)cur, out);
> 
> Staring at fault_in_safe_writeable(), we could also do
> 
> /* Stop once we overflow to 0. */
> end = PAGE_ALIGN(end)
> if (start < end)
> 	end = 0;
> 
> for (cur = start; cur != end; cur = PAGE_ALIGN_DOWN(cur + PAGE_SIZE))
> 	unsafe_put_user(0, (char __user *)cur, out);
> 
> Essentially, removing the "cur" check from the loop condition. Not sure if
> that is better.

The current code is simpler. Your now saying may save the CPU execution
instructions a little bit. Both is fine to me.

I don't have strong preference, I can make v4 to address these concerns
if decided. Thanks for careful checking. 

> 
> In any case, if all functions later look similar and clearer it's a big win.

Agreed.

Re: [PATCH v4 4/4] mm/gup: clean up codes in fault_in_xxx() functions

[David Hildenbrand]

On 11.04.25 13:15, Baoquan He wrote:
> On 04/11/25 at 10:54am, David Hildenbrand wrote:
>> On 10.04.25 05:57, Baoquan He wrote:
>>> The code style in fault_in_readable() and fault_in_writable() is a
>>> little inconsistent with fault_in_safe_writeable(). In fault_in_readable()
>>> and fault_in_writable(), it uses 'uaddr' passed in as loop cursor. While
>>> in fault_in_safe_writeable(), local variable 'start' is used as loop
>>> cursor. This may mislead people when reading code or making change in
>>> these codes.
>>>
>>> Here define explicit loop cursor and use for loop to simplify codes in
>>> these three functions. These cleanup can make them be consistent in
>>> code style and improve readability.
>>>
>>> Signed-off-by: Baoquan He <bhe@redhat.com>
>>> ---
>>>    mm/gup.c | 65 +++++++++++++++++++++++---------------------------------
>>>    1 file changed, 26 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/mm/gup.c b/mm/gup.c
>>> index 77a5bc622567..a76bd7e90a71 100644
>>> --- a/mm/gup.c
>>> +++ b/mm/gup.c
>>> @@ -2113,28 +2113,24 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
>>>     */
>>>    size_t fault_in_writeable(char __user *uaddr, size_t size)
>>>    {
>>> -	char __user *start = uaddr, *end;
>>> +	const unsigned long start = (unsigned long)uaddr;
>>> +	const unsigned long end = start + size;
>>> +	unsigned long cur = start;
>>
>> I would initialize cur in the for loop header, makes the loop easier to
>> read.
> 
> Both is fine to me. It's to satisfy checkpatch.sh which complains about
> exceeding 80 char in the line.

Did checkpatch.sh actually complain? You might be happy to learn that 
the new limit is 100. :)

[...]

>> /* Stop once we overflow to 0. */
>> end = PAGE_ALIGN(end)
>> if (start < end)
>> 	end = 0;
>>
>> for (cur = start; cur != end; cur = PAGE_ALIGN_DOWN(cur + PAGE_SIZE))
>> 	unsafe_put_user(0, (char __user *)cur, out);
>>
>> Essentially, removing the "cur" check from the loop condition. Not sure if
>> that is better.
> 
> The current code is simpler. Your now saying may save the CPU execution
> instructions a little bit. Both is fine to me.
> 
> I don't have strong preference, I can make v4 to address these concerns
> if decided. Thanks for careful checking.

Whatever you prefer!

-- 
Cheers,

David / dhildenb

Re: [PATCH v4 4/4] mm/gup: clean up codes in fault_in_xxx() functions

[Baoquan He]

On 04/11/25 at 01:41pm, David Hildenbrand wrote:
> On 11.04.25 13:15, Baoquan He wrote:
> > On 04/11/25 at 10:54am, David Hildenbrand wrote:
> > > On 10.04.25 05:57, Baoquan He wrote:
> > > > The code style in fault_in_readable() and fault_in_writable() is a
> > > > little inconsistent with fault_in_safe_writeable(). In fault_in_readable()
> > > > and fault_in_writable(), it uses 'uaddr' passed in as loop cursor. While
> > > > in fault_in_safe_writeable(), local variable 'start' is used as loop
> > > > cursor. This may mislead people when reading code or making change in
> > > > these codes.
> > > > 
> > > > Here define explicit loop cursor and use for loop to simplify codes in
> > > > these three functions. These cleanup can make them be consistent in
> > > > code style and improve readability.
> > > > 
> > > > Signed-off-by: Baoquan He <bhe@redhat.com>
> > > > ---
> > > >    mm/gup.c | 65 +++++++++++++++++++++++---------------------------------
> > > >    1 file changed, 26 insertions(+), 39 deletions(-)
> > > > 
> > > > diff --git a/mm/gup.c b/mm/gup.c
> > > > index 77a5bc622567..a76bd7e90a71 100644
> > > > --- a/mm/gup.c
> > > > +++ b/mm/gup.c
> > > > @@ -2113,28 +2113,24 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
> > > >     */
> > > >    size_t fault_in_writeable(char __user *uaddr, size_t size)
> > > >    {
> > > > -	char __user *start = uaddr, *end;
> > > > +	const unsigned long start = (unsigned long)uaddr;
> > > > +	const unsigned long end = start + size;
> > > > +	unsigned long cur = start;
> > > 
> > > I would initialize cur in the for loop header, makes the loop easier to
> > > read.
> > 
> > Both is fine to me. It's to satisfy checkpatch.sh which complains about
> > exceeding 80 char in the line.
> 
> Did checkpatch.sh actually complain? You might be happy to learn that the
> new limit is 100. :)

That's great to know. I never noticed this and always wrap via vim's
indication.

> 
> [...]
> 
> > > /* Stop once we overflow to 0. */
> > > end = PAGE_ALIGN(end)
> > > if (start < end)
> > > 	end = 0;
> > > 
> > > for (cur = start; cur != end; cur = PAGE_ALIGN_DOWN(cur + PAGE_SIZE))
> > > 	unsafe_put_user(0, (char __user *)cur, out);
> > > 
> > > Essentially, removing the "cur" check from the loop condition. Not sure if
> > > that is better.
> > 
> > The current code is simpler. Your now saying may save the CPU execution
> > instructions a little bit. Both is fine to me.
> > 
> > I don't have strong preference, I can make v4 to address these concerns
> > if decided. Thanks for careful checking.
> 
> Whatever you prefer!

Great, will make change in v4. Thx.

Re: [PATCH v4 4/4] mm/gup: clean up codes in fault_in_xxx() functions

[David Hildenbrand]

On 13.04.25 03:07, Baoquan He wrote:
> On 04/11/25 at 01:41pm, David Hildenbrand wrote:
>> On 11.04.25 13:15, Baoquan He wrote:
>>> On 04/11/25 at 10:54am, David Hildenbrand wrote:
>>>> On 10.04.25 05:57, Baoquan He wrote:
>>>>> The code style in fault_in_readable() and fault_in_writable() is a
>>>>> little inconsistent with fault_in_safe_writeable(). In fault_in_readable()
>>>>> and fault_in_writable(), it uses 'uaddr' passed in as loop cursor. While
>>>>> in fault_in_safe_writeable(), local variable 'start' is used as loop
>>>>> cursor. This may mislead people when reading code or making change in
>>>>> these codes.
>>>>>
>>>>> Here define explicit loop cursor and use for loop to simplify codes in
>>>>> these three functions. These cleanup can make them be consistent in
>>>>> code style and improve readability.
>>>>>
>>>>> Signed-off-by: Baoquan He <bhe@redhat.com>
>>>>> ---
>>>>>     mm/gup.c | 65 +++++++++++++++++++++++---------------------------------
>>>>>     1 file changed, 26 insertions(+), 39 deletions(-)
>>>>>
>>>>> diff --git a/mm/gup.c b/mm/gup.c
>>>>> index 77a5bc622567..a76bd7e90a71 100644
>>>>> --- a/mm/gup.c
>>>>> +++ b/mm/gup.c
>>>>> @@ -2113,28 +2113,24 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
>>>>>      */
>>>>>     size_t fault_in_writeable(char __user *uaddr, size_t size)
>>>>>     {
>>>>> -	char __user *start = uaddr, *end;
>>>>> +	const unsigned long start = (unsigned long)uaddr;
>>>>> +	const unsigned long end = start + size;
>>>>> +	unsigned long cur = start;
>>>>
>>>> I would initialize cur in the for loop header, makes the loop easier to
>>>> read.
>>>
>>> Both is fine to me. It's to satisfy checkpatch.sh which complains about
>>> exceeding 80 char in the line.
>>
>> Did checkpatch.sh actually complain? You might be happy to learn that the
>> new limit is 100. :)
> 
> That's great to know. I never noticed this and always wrap via vim's
> indication.

Note that coding style says:

"
The preferred limit on the length of a single line is 80 columns.

Statements longer than 80 columns should be broken into sensible chunks,
unless exceeding 80 columns significantly increases readability and does
not hide information.
"

So 80 is still recommended, but there is nothing wrong about exceeding 
80 if there is good reason.


-- 
Cheers,

David / dhildenb

[PATCH v5 4/4] mm/gup: clean up codes in fault_in_xxx() functions

[Baoquan He]

The code style in fault_in_readable() and fault_in_writable() is a
little inconsistent with fault_in_safe_writeable(). In fault_in_readable()
and fault_in_writable(), it uses 'uaddr' passed in as loop cursor. While
in fault_in_safe_writeable(), local variable 'start' is used as loop
cursor. This may mislead people when reading code or making change in
these codes.

Here define explicit loop cursor and use for loop to simplify codes in
these three functions. These cleanup can make them be consistent in
code style and improve readability.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
v4->v5:
Address minor concerns from David:
- Remove one blank line in fault_in_writable() added in v4;
- Put the loop cursor initialization 'cur = start;' into the for loop
  initialization part.
  
 mm/gup.c | 62 ++++++++++++++++++++++----------------------------------
 1 file changed, 24 insertions(+), 38 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 77a5bc622567..f32168339390 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2113,28 +2113,22 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
  */
 size_t fault_in_writeable(char __user *uaddr, size_t size)
 {
-	char __user *start = uaddr, *end;
+	const unsigned long start = (unsigned long)uaddr;
+	const unsigned long end = start + size;
+	unsigned long cur;
 
 	if (unlikely(size == 0))
 		return 0;
 	if (!user_write_access_begin(uaddr, size))
 		return size;
-	if (!PAGE_ALIGNED(uaddr)) {
-		unsafe_put_user(0, uaddr, out);
-		uaddr = (char __user *)PAGE_ALIGN((unsigned long)uaddr);
-	}
-	end = (char __user *)PAGE_ALIGN((unsigned long)start + size);
-	if (unlikely(end < start))
-		end = NULL;
-	while (uaddr != end) {
-		unsafe_put_user(0, uaddr, out);
-		uaddr += PAGE_SIZE;
-	}
 
+	/* Stop once we overflow to 0. */
+	for (cur = start; cur && cur < end; cur = PAGE_ALIGN_DOWN(cur + PAGE_SIZE))
+		unsafe_put_user(0, (char __user *)cur, out);
 out:
 	user_write_access_end();
-	if (size > uaddr - start)
-		return size - (uaddr - start);
+	if (size > cur - start)
+		return size - (cur - start);
 	return 0;
 }
 EXPORT_SYMBOL(fault_in_writeable);
@@ -2188,26 +2182,24 @@ EXPORT_SYMBOL(fault_in_subpage_writeable);
  */
 size_t fault_in_safe_writeable(const char __user *uaddr, size_t size)
 {
-	unsigned long start = (unsigned long)uaddr, end;
+	const unsigned long start = (unsigned long)uaddr;
+	const unsigned long end = start + size;
+	unsigned long cur;
 	struct mm_struct *mm = current->mm;
 	bool unlocked = false;
 
 	if (unlikely(size == 0))
 		return 0;
-	end = PAGE_ALIGN(start + size);
-	if (end < start)
-		end = 0;
 
 	mmap_read_lock(mm);
-	do {
-		if (fixup_user_fault(mm, start, FAULT_FLAG_WRITE, &unlocked))
+	/* Stop once we overflow to 0. */
+	for (cur = start; cur && cur < end; cur = PAGE_ALIGN_DOWN(cur + PAGE_SIZE))
+		if (fixup_user_fault(mm, cur, FAULT_FLAG_WRITE, &unlocked))
 			break;
-		start = (start + PAGE_SIZE) & PAGE_MASK;
-	} while (start != end);
 	mmap_read_unlock(mm);
 
-	if (size > start - (unsigned long)uaddr)
-		return size - (start - (unsigned long)uaddr);
+	if (size > cur - start)
+		return size - (cur - start);
 	return 0;
 }
 EXPORT_SYMBOL(fault_in_safe_writeable);
@@ -2222,30 +2214,24 @@ EXPORT_SYMBOL(fault_in_safe_writeable);
  */
 size_t fault_in_readable(const char __user *uaddr, size_t size)
 {
-	const char __user *start = uaddr, *end;
+	const unsigned long start = (unsigned long)uaddr;
+	const unsigned long end = start + size;
+	unsigned long cur;
 	volatile char c;
 
 	if (unlikely(size == 0))
 		return 0;
 	if (!user_read_access_begin(uaddr, size))
 		return size;
-	if (!PAGE_ALIGNED(uaddr)) {
-		unsafe_get_user(c, uaddr, out);
-		uaddr = (const char __user *)PAGE_ALIGN((unsigned long)uaddr);
-	}
-	end = (const char __user *)PAGE_ALIGN((unsigned long)start + size);
-	if (unlikely(end < start))
-		end = NULL;
-	while (uaddr != end) {
-		unsafe_get_user(c, uaddr, out);
-		uaddr += PAGE_SIZE;
-	}
 
+	/* Stop once we overflow to 0. */
+	for (cur = start; cur && cur < end; cur = PAGE_ALIGN_DOWN(cur + PAGE_SIZE))
+		unsafe_get_user(c, (const char __user *)cur, out);
 out:
 	user_read_access_end();
 	(void)c;
-	if (size > uaddr - start)
-		return size - (uaddr - start);
+	if (size > cur - start)
+		return size - (cur - start);
 	return 0;
 }
 EXPORT_SYMBOL(fault_in_readable);
-- 
2.41.0

Re: [PATCH v5 4/4] mm/gup: clean up codes in fault_in_xxx() functions

[David Hildenbrand]

On 13.04.25 04:04, Baoquan He wrote:
> The code style in fault_in_readable() and fault_in_writable() is a
> little inconsistent with fault_in_safe_writeable(). In fault_in_readable()
> and fault_in_writable(), it uses 'uaddr' passed in as loop cursor. While
> in fault_in_safe_writeable(), local variable 'start' is used as loop
> cursor. This may mislead people when reading code or making change in
> these codes.
> 
> Here define explicit loop cursor and use for loop to simplify codes in
> these three functions. These cleanup can make them be consistent in
> code style and improve readability.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---

Hopefully we don't introduce anything unexpected ... do we have some 
unit test that could make use feel better, especially regarding end < start?

If not, could we add one based on some feature that ends up calling at 
least one of these functions?

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

-- 
Cheers,

David / dhildenb

Re: [PATCH v5 4/4] mm/gup: clean up codes in fault_in_xxx() functions

[Baoquan He]

On 04/13/25 at 10:09pm, David Hildenbrand wrote:
> On 13.04.25 04:04, Baoquan He wrote:
> > The code style in fault_in_readable() and fault_in_writable() is a
> > little inconsistent with fault_in_safe_writeable(). In fault_in_readable()
> > and fault_in_writable(), it uses 'uaddr' passed in as loop cursor. While
> > in fault_in_safe_writeable(), local variable 'start' is used as loop
> > cursor. This may mislead people when reading code or making change in
> > these codes.
> > 
> > Here define explicit loop cursor and use for loop to simplify codes in
> > these three functions. These cleanup can make them be consistent in
> > code style and improve readability.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> 
> Hopefully we don't introduce anything unexpected ... do we have some unit
> test that could make use feel better, especially regarding end < start?
> 
> If not, could we add one based on some feature that ends up calling at least
> one of these functions?

Seems no existing case. GUP has selftests, no test codes for kunit. I will see
if I can add one, maybe it's not easy.

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

Thanks.