Re: [PATCH v1 3/3] mm: merge folio_is_secretmem() into folio_fast_pin_allowed()

[Mike Rapoport]

On Mon, Mar 25, 2024 at 02:41:14PM +0100, David Hildenbrand wrote:
> folio_is_secretmem() is currently only used during GUP-fast, and using
> it in wrong context where concurrent truncation might happen, could be
> problematic.
> 
> Nowadays, folio_fast_pin_allowed() performs similar checks during
> GUP-fast and contains a lot of careful handling -- READ_ONCE( -- ), sanity
> checks -- lockdep_assert_irqs_disabled() --  and helpful comments on how
> this handling is safe and correct.
> 
> So let's merge folio_is_secretmem() into folio_fast_pin_allowed(), still
> avoiding checking the actual mapping only if really required.
> 
> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>

Reviewed-by: Mike Rapoport (IBM) <rppt@xxxxxxxxxx>

A few comments below, no strong feelings about them.

> ---
>  include/linux/secretmem.h | 21 ++-------------------
>  mm/gup.c                  | 33 +++++++++++++++++++++------------
>  2 files changed, 23 insertions(+), 31 deletions(-)
> 
> diff --git a/include/linux/secretmem.h b/include/linux/secretmem.h
> index 6996f1f53f14..e918f96881f5 100644
> --- a/include/linux/secretmem.h
> +++ b/include/linux/secretmem.h
> @@ -6,25 +6,8 @@
>  
>  extern const struct address_space_operations secretmem_aops;
>  
> -static inline bool folio_is_secretmem(struct folio *folio)
> +static inline bool secretmem_mapping(struct address_space *mapping)
>  {
> -	struct address_space *mapping;
> -
> -	/*
> -	 * Using folio_mapping() is quite slow because of the actual call
> -	 * instruction.
> -	 * We know that secretmem pages are not compound and LRU so we can
> -	 * save a couple of cycles here.
> -	 */
> -	if (folio_test_large(folio) || folio_test_lru(folio))
> -		return false;
> -
> -	mapping = (struct address_space *)
> -		((unsigned long)folio->mapping & ~PAGE_MAPPING_FLAGS);
> -
> -	if (!mapping || mapping != folio->mapping)
> -		return false;
> -
>  	return mapping->a_ops == &secretmem_aops;
>  }
>  
> @@ -38,7 +21,7 @@ static inline bool vma_is_secretmem(struct vm_area_struct *vma)
>  	return false;
>  }
>  
> -static inline bool folio_is_secretmem(struct folio *folio)
> +static inline bool secretmem_mapping(struct address_space *mapping)
>  {
>  	return false;
>  }
> diff --git a/mm/gup.c b/mm/gup.c
> index e7510b6ce765..69d8bc8e4451 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2472,6 +2472,8 @@ EXPORT_SYMBOL(get_user_pages_unlocked);
>   * This call assumes the caller has pinned the folio, that the lowest page table
>   * level still points to this folio, and that interrupts have been disabled.
>   *
> + * GUP-fast must reject all secretmem folios.
> + *
>   * Writing to pinned file-backed dirty tracked folios is inherently problematic
>   * (see comment describing the writable_file_mapping_allowed() function). We
>   * therefore try to avoid the most egregious case of a long-term mapping doing
> @@ -2484,22 +2486,32 @@ EXPORT_SYMBOL(get_user_pages_unlocked);
>  static bool folio_fast_pin_allowed(struct folio *folio, unsigned int flags)

Now when this function checks for gup in general, maybe it's worth to
rename it to, say, folio_fast_gup_allowed.

>  {
>  	struct address_space *mapping;
> +	bool check_secretmem = false;
> +	bool reject_file_backed = false;
>  	unsigned long mapping_flags;
>  
>  	/*
>  	 * If we aren't pinning then no problematic write can occur. A long term
>  	 * pin is the most egregious case so this is the one we disallow.
>  	 */
> -	if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) !=
> +	if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) ==
>  	    (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE))
> -		return true;
> +		reject_file_backed = true;
> +
> +	/* We hold a folio reference, so we can safely access folio fields. */
>  
> -	/* The folio is pinned, so we can safely access folio fields. */
> +	/* secretmem folios are only order-0 folios and never LRU folios. */

Nit:                           ^ always

> +	if (IS_ENABLED(CONFIG_SECRETMEM) && !folio_test_large(folio) &&
> +	    !folio_test_lru(folio))
> +		check_secretmem = true;
> +
> +	if (!reject_file_backed && !check_secretmem)
> +		return true;
>  
>  	if (WARN_ON_ONCE(folio_test_slab(folio)))
>  		return false;
>  
> -	/* hugetlb mappings do not require dirty-tracking. */
> +	/* hugetlb neither requires dirty-tracking nor can be secretmem. */
>  	if (folio_test_hugetlb(folio))
>  		return true;
>  
> @@ -2535,10 +2547,12 @@ static bool folio_fast_pin_allowed(struct folio *folio, unsigned int flags)
>  
>  	/*
>  	 * At this point, we know the mapping is non-null and points to an
> -	 * address_space object. The only remaining whitelisted file system is
> -	 * shmem.
> +	 * address_space object.
>  	 */
> -	return shmem_mapping(mapping);
> +	if (check_secretmem && secretmem_mapping(mapping))
> +		return false;
> +	/* The only remaining allowed file system is shmem. */
> +	return !reject_file_backed || shmem_mapping(mapping);
>  }
>  
>  static void __maybe_unused undo_dev_pagemap(int *nr, int nr_start,
> @@ -2624,11 +2638,6 @@ static int gup_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr,
>  		if (!folio)
>  			goto pte_unmap;
>  
> -		if (unlikely(folio_is_secretmem(folio))) {
> -			gup_put_folio(folio, 1, flags);
> -			goto pte_unmap;
> -		}
> -
>  		if (unlikely(pmd_val(pmd) != pmd_val(*pmdp)) ||
>  		    unlikely(pte_val(pte) != pte_val(ptep_get(ptep)))) {
>  			gup_put_folio(folio, 1, flags);
> -- 
> 2.43.2
> 

-- 
Sincerely yours,
Mike.