Re: [PATCHv5 2/3] mm/gup: fix omission of check on FOLL_LONGTERM in gup fast path

[Jason Gunthorpe]

On Fri, Feb 28, 2020 at 07:32:29PM +0800, Pingfan Liu wrote:
> FOLL_LONGTERM suggests a pin which is going to be given to hardware and
> can't move. It would truncate CMA permanently and should be excluded.
> 
> FOLL_LONGTERM has already been checked in the slow path, but not checked in
> the fast path, which means a possible leak of CMA page to longterm pinned
> requirement through this crack.
> 
> Place a check in try_get_compound_head() in the fast path.
> 
> Some note about the check:
> Huge page's subpages have the same migrate type due to either
> allocation from a free_list[] or alloc_contig_range() with param
> MIGRATE_MOVABLE. So it is enough to check on a single subpage
> by is_migrate_cma_page(subpage)
> 
> Signed-off-by: Pingfan Liu <kernelfans@xxxxxxxxx>
> Cc: Ira Weiny <ira.weiny@xxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Mike Rapoport <rppt@xxxxxxxxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Cc: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> Cc: John Hubbard <jhubbard@xxxxxxxxxx>
> Cc: "Aneesh Kumar K.V" <aneesh.kumar@xxxxxxxxxxxxx>
> Cc: Keith Busch <keith.busch@xxxxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> Cc: Shuah Khan <shuah@xxxxxxxxxx>
> To: linux-mm@xxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
>  mm/gup.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index cd8075e..f0d6804 100644
> +++ b/mm/gup.c
> @@ -33,9 +33,21 @@ struct follow_page_context {
>   * Return the compound head page with ref appropriately incremented,
>   * or NULL if that failed.
>   */
> -static inline struct page *try_get_compound_head(struct page *page, int refs)
> +static inline struct page *try_get_compound_head(struct page *page, int refs,
> +	unsigned int flags)
>  {
> -	struct page *head = compound_head(page);
> +	struct page *head;
> +
> +	/*
> +	 * Huge page's subpages have the same migrate type due to either
> +	 * allocation from a free_list[] or alloc_contig_range() with param
> +	 * MIGRATE_MOVABLE. So it is enough to check on a single subpage.
> +	 */
> +	if (unlikely(flags & FOLL_LONGTERM) &&
> +		is_migrate_cma_page(page))
> +		return NULL;

This doesn't seem very good actually.

If I understand properly, if the system has randomly decided to place,
say, an anonymous page in a CMA region when an application did mmap(),
then when the application tries to use this page with a LONGTERM pin
it gets an immediate failure because of the above.

This not OK - the application should not be subject to random failures
related to long term pins beyond its direct control.

Essentially, failures should only originate from the application using
specific mmap scenarios, not randomly based on something the MM did,
and certainly never for anonymous memory.

I think the correct action here is to trigger migration of the page so
it is not in CMA.

Jason