| Message ID | 20250209174711.60889-1-david.laight.linux@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] mm: Remove the access_ok() call from gup_fast_fallback(). | expand |
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
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
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
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>
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)
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(-)