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

[David Hildenbrand <david@redhat.com>]

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.


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