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

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

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?

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.

>

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

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.

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