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

[Mike Rapoport <rppt@kernel.org>]

On Tue, Jan 17, 2023 at 11:58:34AM -0400, 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

...

> -static bool is_valid_gup_flags(unsigned int gup_flags)
> +/*
> + * Check that the given flags are valid for the exported gup/pup interface, and
> + * update them with the required flags that the caller must have set.
> + */
> +static bool is_valid_gup_args(struct page **pages, struct vm_area_struct **vmas,
> +			      int *locked, unsigned int *gup_flags_p,
> +			      unsigned int to_set)
>  {
> +	unsigned int gup_flags = *gup_flags_p;
> +
>  	/*
> -	 * FOLL_PIN must only be set internally by the pin_user_pages*() APIs,
> -	 * never directly by the caller, so enforce that with an assertion:
> +	 * These flags not allowed to be specified externally to the gup
> +	 * interfaces:
> +	 * - FOLL_PIN/FOLL_TRIED/FOLL_FAST_ONLY is internal only

                                              ^ are?

> +	 * - FOLL_REMOTE is internal only and used on follow_page()
>  	 */
> -	if (WARN_ON_ONCE(gup_flags & FOLL_PIN))
> +	if (WARN_ON_ONCE(gup_flags & (FOLL_PIN | FOLL_TRIED |
> +				      FOLL_REMOTE | FOLL_FAST_ONLY)))
>  		return false;
> +
> +	gup_flags |= to_set;
> +
> +	/* FOLL_GET and FOLL_PIN are mutually exclusive. */
> +	if (WARN_ON_ONCE((gup_flags & (FOLL_PIN | FOLL_GET)) ==
> +			 (FOLL_PIN | FOLL_GET)))
> +		return false;
> +

-- 
Sincerely yours,
Mike.