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

[Baoquan He]

On 04/08/25 at 12:00pm, David Hildenbrand wrote:
> On 08.04.25 11:52, David Hildenbrand wrote:
> > On 07.04.25 05:03, 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'.
> > > 
> > > Here fix the code bug in fault_in_safe_writeable(), and also adjusting
> > > the codes in fault_in_readable() and fault_in_writeable() to use local
> > > variable 'start' to loop so that codes in these three functions are
> > > consistent.
> > > 
> > 
> > I probably phrased it poorly in my other reply: the confusing part (to
> > me) is adjusting "start". Maybe we should have unsigned long start,end,cur;
> > 
> > Maybe we should really split the "fix" from the cleanups, and tag the
> > fix with a Fixes:.

> > 
> > I was wondering if these functions could be simplified a bit. But the
> > overflow handling is a bit nasty.
> 
> FWIW, maybe the following could work and clarify things. Just a thought.

The code simplification looks great to me. I will make a patch to only
contains the code bug fixing with Fixes so that it's eaiser to back port
to stable kernel, and make another patch as below to refactor codes in
fault_in_readable/writable/safe_writable(). Thanks for suggestion.

> 
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 92351e2fa876b..7a3f78a209f8b 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2223,30 +2223,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;
>         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;
> -       }
> -
> -out:
> +       /* 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);
>         user_read_access_end();
>         (void)c;
> -       if (size > uaddr - start)
> -               return size - (uaddr - start);
> +out:
> +       if (size > cur - start)
> +               return size - (cur - start);
>         return 0;
>  }
>  EXPORT_SYMBOL(fault_in_readable);
> 
> 
> -- 
> Cheers,
> 
> David / dhildenb
>