[PATCH 1/1] mm: Remove the access_ok() call from gup_fast_fallback().

[PATCH 1/1] mm: Remove the access_ok() call from gup_fast_fallback().

[David Laight]

Historiaclly the code relied on access_ok() to validate the address range.
Commit 26f4c328079d7 added an explicit wrap check before access_ok().
Commit c28b1fc70390d then changed the wrap test to use check_add_overflow().
Commit 6014bc27561f2 relaxed the checks in x86-64's access_ok() and added
  an explicit check for TASK_SIZE here to make up for it.
That left a pointless access_ok() call with its associated 'lfence' that
  can never actually fail.
So just delete the test.

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---
 mm/gup.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 3883b307780e..79a3d2228bf9 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2757,7 +2757,7 @@ EXPORT_SYMBOL(get_user_pages_unlocked);
  *
  *  *) ptes can be read atomically by the architecture.
  *
- *  *) access_ok is sufficient to validate userspace address ranges.
+ *  *) valid user addesses are below TASK_MAX_SIZE
  *
  * The last two assumptions can be relaxed by the addition of helper functions.
  *
@@ -3411,8 +3411,6 @@ static int gup_fast_fallback(unsigned long start, unsigned long nr_pages,
 		return -EOVERFLOW;
 	if (end > TASK_SIZE_MAX)
 		return -EFAULT;
-	if (unlikely(!access_ok((void __user *)start, len)))
-		return -EFAULT;
 
 	nr_pinned = gup_fast(start, end, gup_flags, pages);
 	if (nr_pinned == nr_pages || gup_flags & FOLL_FAST_ONLY)
-- 
2.39.5

Re: [PATCH 1/1] mm: Remove the access_ok() call from gup_fast_fallback().

[Jason Gunthorpe]

On Sun, Feb 09, 2025 at 05:47:11PM +0000, David Laight wrote:
> Historiaclly the code relied on access_ok() to validate the address range.
> Commit 26f4c328079d7 added an explicit wrap check before access_ok().
> Commit c28b1fc70390d then changed the wrap test to use check_add_overflow().
> Commit 6014bc27561f2 relaxed the checks in x86-64's access_ok() and added
>   an explicit check for TASK_SIZE here to make up for it.
> That left a pointless access_ok() call with its associated 'lfence' that
>   can never actually fail.
> So just delete the test.
> 
> Signed-off-by: David Laight <david.laight.linux@gmail.com>
> ---
>  mm/gup.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

I often wonder about about access_ok() calls, if they still do
anything..

Jason

Re: [PATCH 1/1] mm: Remove the access_ok() call from gup_fast_fallback().

[David Laight]

On Sun, 9 Feb 2025 14:24:22 -0400
Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Sun, Feb 09, 2025 at 05:47:11PM +0000, David Laight wrote:
> > Historiaclly the code relied on access_ok() to validate the address range.
> > Commit 26f4c328079d7 added an explicit wrap check before access_ok().
> > Commit c28b1fc70390d then changed the wrap test to use check_add_overflow().
> > Commit 6014bc27561f2 relaxed the checks in x86-64's access_ok() and added
> >   an explicit check for TASK_SIZE here to make up for it.
> > That left a pointless access_ok() call with its associated 'lfence' that
> >   can never actually fail.
> > So just delete the test.
> > 
> > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > ---
> >  mm/gup.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)  
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> I often wonder about about access_ok() calls, if they still do
> anything..

They still do 'stuff' and end up containing a slow memory synchronising
instruction (to avoid speculative accesses controlled by the application).

But there are better ways to handle bad user pointers.
So, mostly access_ok() isn't needed outside the architecture code
that handles userspace accesses.

	David

Re: [PATCH 1/1] mm: Remove the access_ok() call from gup_fast_fallback().

[Linus Torvalds]

On Sun, 9 Feb 2025 at 11:00, David Laight <david.laight.linux@gmail.com> wrote:
>
> They still do 'stuff' and end up containing a slow memory synchronising
> instruction (to avoid speculative accesses controlled by the application).
>
> But there are better ways to handle bad user pointers.

Not always. If you use __get_user() and friends, you basically *have*
to use access_ok(), or you have to be playing games (like some tracing
code does, which actually wants to use it as a "I want a kernel
pointer _or_ a user pointer{".

Now, admittedly probably nobody should be using __get_user() and
friends any more. Almost all the reasons for using it are entirely
historical and just not true any more.

But also, comparing against TASK_SIZE_MAX isn't actually the same as
access_ok() historically. We've moved in that direction, yes, but we
very much used to have a distinction between "this is the fixed
maximum", and "this is the actual run-time size".

We've moved towards just using TASK_SIZE_MAX mainly because the
run-time size check was annoyingly expensive is some pretty critical
code.

But historically the TASK_SIZE_MAX thing is the "this is fast but not
exact, use it only for special code that knows what it is doing", and
"access_ok()" was "this is proper"

> So, mostly access_ok() isn't needed outside the architecture code
> that handles userspace accesses.

Oh, there is still a fair amount of code that really does need it.

Admittedly most of it should either be converted to just using regular
get/put_user(), or into the modern "user_access_begin()" model, but we
do have a number of __get/put_user() users that still very much need
that access_ok().

And anything that follows page tables had better check that it's
proper. But in that case, I do believe checking for TASK_SIZE_MAX
tends to be equivalent.

              Linus

Re: [PATCH 1/1] mm: Remove the access_ok() call from gup_fast_fallback().

[David Hildenbrand]

On 09.02.25 18:47, David Laight wrote:
> Historiaclly the code relied on access_ok() to validate the address range.
> Commit 26f4c328079d7 added an explicit wrap check before access_ok().
> Commit c28b1fc70390d then changed the wrap test to use check_add_overflow().
> Commit 6014bc27561f2 relaxed the checks in x86-64's access_ok() and added

checkpatch.pl will correctly tell you about the wrongly quoted commit ids.

E.g.,

ERROR: Please use git commit description style 'commit <12+ chars of 
sha1> ("<title line>")' - ie: 'Commit 26f4c328079d ("mm: simplify 
gup_fast_permitted")'


Apart from that

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

-- 
Cheers,

David / dhildenb