On 1/17/23 07:58, Jason Gunthorpe wrote:
The GUP family of functions have a complex, but fairly well defined, set
of invariants for their arguments. Currently these are sprinkled about,
sometimes in duplicate through many functions.
Internally we don't follow all the invariants that the external interface
has to follow, so place these checks directly at the exported
interface. This ensures the internal functions never reach a violated
invariant.
Remove the duplicated invariant checks.
The end result is to make these functions fully internal:
__get_user_pages_locked()
internal_get_user_pages_fast()
__gup_longterm_locked()
And all the other functions call directly into one of these.
Suggested-by: John Hubbard <jhubbard@xxxxxxxxxx>
Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
---
mm/gup.c | 150 +++++++++++++++++++++++------------------------
mm/huge_memory.c | 10 ----
2 files changed, 75 insertions(+), 85 deletions(-)
diff --git a/mm/gup.c b/mm/gup.c
index 2c833f862d0354..9e332e3f6ea8e2 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -215,7 +215,6 @@ int __must_check try_grab_page(struct page *page, unsigned int flags)
{
struct folio *folio = page_folio(page);
- WARN_ON_ONCE((flags & (FOLL_GET | FOLL_PIN)) == (FOLL_GET | FOLL_PIN));
try_grab_page() is declared in mm.h and is therefore potentially
something that other subsystems could call--although they really
shouldn't! And here, we are simply assuming that that is enough. But in
order to be really comfortable removing this check on the basis of
"try_grab_page() is internal to mm", I think it would help to move its
declaration from mm.h, to mm/internal.h. Perhaps as a separate patch.
if (WARN_ON_ONCE(folio_ref_count(folio) <= 0))
return -ENOMEM;
@@ -818,7 +817,7 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address,
if (vma_is_secretmem(vma))
return NULL;
- if (foll_flags & FOLL_PIN)
+ if (WARN_ON_ONCE(foll_flags & FOLL_PIN))
OK, so we're slightly fortifying follow_page() checking, but
not at the level of is_valid_gup_args(). Should this be mentioned
in the commit description? And should the checks be more extensive?
...
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index abe6cfd92ffa0e..eaf879c835de44 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1039,11 +1039,6 @@ struct page *follow_devmap_pmd(struct vm_area_struct *vma, unsigned long addr,
assert_spin_locked(pmd_lockptr(mm, pmd));
- /* FOLL_GET and FOLL_PIN are mutually exclusive. */
- if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) ==
- (FOLL_PIN | FOLL_GET)))
- return NULL;
-
For both follow_devmap_pmd() and follow_devmap_pud(), below, it looks like
the following external API path is left exposed (with respect to checking
gup flags):
do_mlock()
__mm_populate()
populate_vma_page_range()
__get_user_pages()
follow_page_mask()
...
follow_devmap_pmd()
So I'm not sure that it's good to delete these checks without covering
that path.
if (flags & FOLL_WRITE && !pmd_write(*pmd))
return NULL;
@@ -1202,11 +1197,6 @@ struct page *follow_devmap_pud(struct vm_area_struct *vma, unsigned long addr,
if (flags & FOLL_WRITE && !pud_write(*pud))
return NULL;
- /* FOLL_GET and FOLL_PIN are mutually exclusive. */
- if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) ==
- (FOLL_PIN | FOLL_GET)))
- return NULL;
-
if (pud_present(*pud) && pud_devmap(*pud))
/* pass */;
else
thanks,
--
John Hubbard
NVIDIA
