mm/gup.c: Handle error at earliest for incorrect nr_pages value
Subsystems (2)
PATCH 1/1 mm/gup.c: Handle error at earliest for incorrect nr_pages value
As per documentation, pin_user_pages_fast() & get_user_pages_fast() will return 0, if nr_pages <= 0. But this can be figure out only after going inside the internal_get_user_pages_fast().
This can be handled early. Adding a check for the same.
--- mm/gup.c | 4 ++++ 1 file changed, 4 insertions(+)
mm/gup.c +4 -0
@@ -2817,6 +2817,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages,*/ if (WARN_ON_ONCE (gup_flags &FOLL_PIN ))return -EINVAL ; +if (nr_pages <=0 ) +return 0 ;/* * The caller may or may not have explicitly set FOLL_GET; either way is
@@ -2854,6 +2856,8 @@ int pin_user_pages_fast(unsigned long start, int nr_pages,/* FOLL_GET and FOLL_PIN are mutually exclusive. */ if (WARN_ON_ONCE (gup_flags &FOLL_GET ))return -EINVAL ; +if (nr_pages <=0 ) +return 0 ; gup_flags |=FOLL_PIN ;return internal_get_user_pages_fast (start, nr_pages, gup_flags, pages);
Signature · 1 line
1.9.1
Andrew Morton
maintainerQuoted 5 lines · On Fri, 1 May 2020 01:41:58 +0530 Souptick Joarder <[email protected]>…
As per documentation,
pin_user_pages_fast()&get_user_pages_fast()will return 0, ifnr_pages<= 0. But this can be figure out only after going inside theinternal_get_user_pages_fast().This can be handled early. Adding a check for the same.
Calling get_user_pages() for (nrpages <= 0) is a nonsensical thing to do so it isn't a thing we should care to optimize. Adding new checks will actually slow down the use cases which we *do* care about. And it adds more code.
Quoted 3 lines · Souptick Joarder
As per documentation,
pin_user_pages_fast()&get_user_pages_fast()will return 0, ifnr_pages<= 0. But this can be figure out only after going inside theinternal_get_user_pages_fast().
Why is nr_pages not unsigned? I seem to have convinced myself before that there was a good reason for it but really what is the point of calling either of these functions with nr_pages not > 0?
Quoted 13 lines
This can be handled early. Adding a check for the same.
Signed-off-by Souptick Joarder <[email protected]>--- mm/gup.c | 4 ++++ 1 file changed, 4 insertions(+)mm/gup.c +0
-0
mm/gup.c int get_user_pages_fast(unsigned long start, int nr_pages, · line 2821
Quoted 5 lines
mm/gup.c +2
-0@@ -2817,6 +2817,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages,*/ if (WARN_ON_ONCE (gup_flags &FOLL_PIN ))return -EINVAL ; +if (nr_pages <=0 ) +return 0 ;
I think the documentation may be wrong here... Is there a caller who expects a return of 0 for this behavior?
It seems like these should be a warn on and return -EINVAL. I just don't see the use case here.
Ira
Quoted 16 lines
mm/gup.c +2
-0/* * The caller may or may not have explicitly set FOLL_GET; either way is @@ -2854,6 +2856,8 @@ int pin_user_pages_fast(unsigned long start, int nr_pages,/* FOLL_GET and FOLL_PIN are mutually exclusive. */ if (WARN_ON_ONCE (gup_flags &FOLL_GET ))return -EINVAL ; +if (nr_pages <=0 ) +return 0 ; gup_flags |=FOLL_PIN ;return internal_get_user_pages_fast (start, nr_pages, gup_flags, pages);Signature · 1 line
1.9.1
Quoted 7 lines · Ira Weiny
Quoted 3 lines · Souptick Joarder
As per documentation,
pin_user_pages_fast()&get_user_pages_fast()will return 0, ifnr_pages<= 0. But this can be figure out only after going inside theinternal_get_user_pages_fast().Why is
nr_pagesnot unsigned?
Not sure of why but it can be unsigned.
Quoted 9 lines
Quoted 9 lines
This can be handled early. Adding a check for the same.
Signed-off-by Souptick Joarder <[email protected]>--- mm/gup.c | 4 ++++ 1 file changed, 4 insertions(+)
Quoted 14 lines
mm/gup.c int get_user_pages_fast(unsigned long start, int nr_pages, · line 2821
Quoted 10 lines
mm/gup.c +2
-0@@ -2817,6 +2817,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages,*/ if (WARN_ON_ONCE (gup_flags &FOLL_PIN ))return -EINVAL ; +if (nr_pages <=0 ) +return 0 ;I think the documentation may be wrong here... Is there a caller who expects a return of 0 for this behavior?
My understanding is -
{get/pin}_user_pages_fast() -> internal_get_user_pages_fast()
Inside internal_get_user_pages_fast()
...
start = untagged_addr(start) & PAGE_MASK;
addr = start;
len = (unsigned long) nr_pages << PAGE_SHIFT;
end = start + len;
if (end <= start)
return 0;
...
This is the only place where {get/pin}_user_pages_fast() returns 0 which indirectly checks for nr_pages <= 0. For each call to {get/pin}_user_pages_fast()
we end up checking *nr_pages <= 0* anyway.
There are some instances where caller of these APIs handles return value 0 as well. Example - arch/ia64/kernel/err_inject.c#L145 arch/powerpc/kvm/book3s_64_mmu_hv.c#L582 arch/powerpc/kvm/book3s_64_mmu_hv.c#L1174 drivers/staging/gasket/gasket_page_table.c#L489 drivers/rapidio/devices/rio_mport_cdev.c#L865 drivers/platform/goldfish/goldfish_pipe.c#L277 drivers/gpu/drm/via/via_dmablit.c#L242
(some of the error handling looks incorrect)
If we end up checking *nr_pages <= 0* inside internal_get_user_pages_fast(), then why not at first place like this patch does ?
2nd opinion is, inside internal_get_user_pages_fast(), for *nr_pages <= 0* better
to return -ERRNO rather than 0 ?
John Hubbard
reviewerQuoted 5 lines · Souptick Joarder
As per documentation,
pin_user_pages_fast()&get_user_pages_fast()will return 0, ifnr_pages<= 0. But this can be figure out only after going inside theinternal_get_user_pages_fast().This can be handled early. Adding a check for the same.
Please, no. For precisely the reasons that Andrew gave: you are attempting to optimize for a case that doesn't matter other than for error handling, and which is already handled adequately. And as he also pointed out, it very slightly UN-optimizes the path that we *do* care about. So why are you still advocating for this?
If you want to change the gup/pup API so that asking for zero pages means -EINVAL, then fine, go for it. That's a large thing, and a tree-wide audit, but if you feel it's worth pursuing then it's at least consistent.
But this patch here needs to be abandoned.
thanks,
Signature · 2 lines
John Hubbard NVIDIA
Quoted 29 lines
Signed-off-by Souptick Joarder <[email protected]>--- mm/gup.c | 4 ++++ 1 file changed, 4 insertions(+)mm/gup.c +4
-0@@ -2817,6 +2817,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages,*/ if (WARN_ON_ONCE (gup_flags &FOLL_PIN ))return -EINVAL ; +if (nr_pages <=0 ) +return 0 ;/* * The caller may or may not have explicitly set FOLL_GET; either way is @@ -2854,6 +2856,8 @@ int pin_user_pages_fast(unsigned long start, int nr_pages,/* FOLL_GET and FOLL_PIN are mutually exclusive. */ if (WARN_ON_ONCE (gup_flags &FOLL_GET ))return -EINVAL ; +if (nr_pages <=0 ) +return 0 ; gup_flags |=FOLL_PIN ;return internal_get_user_pages_fast (start, nr_pages, gup_flags, pages);
Quoted 15 lines · John Hubbard
Quoted 5 lines · Souptick Joarder
As per documentation,
pin_user_pages_fast()&get_user_pages_fast()will return 0, ifnr_pages<= 0. But this can be figure out only after going inside theinternal_get_user_pages_fast().This can be handled early. Adding a check for the same.
Please, no. For precisely the reasons that Andrew gave: you are attempting to optimize for a case that doesn't matter other than for error handling, and which is already handled adequately. And as he also pointed out, it very slightly UN-optimizes the path that we *do* care about. So why are you still advocating for this?
Quoted 6 lines
If you want to change the gup/pup API so that asking for zero pages means
-EINVAL, then fine, go for it. That's a large thing, and a tree-wide audit, but if you feel it's worth pursuing then it's at least consistent.But this patch here needs to be abandoned.
Sure, will drop this patch.
Quoted 37 lines
thanks,
--
John Hubbard
NVIDIAQuoted 29 lines
Signed-off-by Souptick Joarder <[email protected]>--- mm/gup.c | 4 ++++ 1 file changed, 4 insertions(+)mm/gup.c +4
-0@@ -2817,6 +2817,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages,*/ if (WARN_ON_ONCE (gup_flags &FOLL_PIN ))return -EINVAL ; +if (nr_pages <=0 ) +return 0 ;/* * The caller may or may not have explicitly set FOLL_GET; either way is @@ -2854,6 +2856,8 @@ int pin_user_pages_fast(unsigned long start, int nr_pages,/* FOLL_GET and FOLL_PIN are mutually exclusive. */ if (WARN_ON_ONCE (gup_flags &FOLL_GET ))return -EINVAL ; +if (nr_pages <=0 ) +return 0 ; gup_flags |=FOLL_PIN ;return internal_get_user_pages_fast (start, nr_pages, gup_flags, pages);