Re: [PATCH 3/8] mm/gup: simplify the external interface functions and consolidate invariants

[John Hubbard <jhubbard@nvidia.com>]

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@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   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