Re: [PATCH V1 3/5] mm/hugetlb: fix memfd_pin_folios resv_huge_pages leak

[Steven Sistare <steven.sistare@oracle.com>]

On 9/3/2024 9:04 PM, Kasireddy, Vivek wrote:
> Hi Steve,
> 
>> Subject: [PATCH V1 3/5] mm/hugetlb: fix memfd_pin_folios resv_huge_pages
>> leak
>>
>> memfd_pin_folios followed by unpin_folios leaves resv_huge_pages
>> elevated
>> if the pages were not already faulted in.  During a normal page fault,
>> resv_huge_pages is consumed here:
>>
>> hugetlb_fault()
>>    alloc_hugetlb_folio()
>>      dequeue_hugetlb_folio_vma()
>>        dequeue_hugetlb_folio_nodemask()
>>          dequeue_hugetlb_folio_node_exact()
>>            free_huge_pages--
>>        resv_huge_pages--
>>
>> During memfd_pin_folios, the page is created by calling
>> alloc_hugetlb_folio_nodemask instead of alloc_hugetlb_folio, and
>> resv_huge_pages is not modified:
>>
>> memfd_alloc_folio()
>>    alloc_hugetlb_folio_nodemask()
>>      dequeue_hugetlb_folio_nodemask()
>>        dequeue_hugetlb_folio_node_exact()
>>          free_huge_pages--
>>
>> alloc_hugetlb_folio_nodemask has other callers that must not modify
>> resv_huge_pages.  Therefore, to fix, define an alternate version of
>> alloc_hugetlb_folio_nodemask for this call site that adjusts
>> resv_huge_pages.
>>
>> Fixes: 89c1905d9c14 ("mm/gup: introduce memfd_pin_folios() for pinning
>> memfd folios")
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>   include/linux/hugetlb.h | 10 ++++++++++
>>   mm/hugetlb.c            | 17 +++++++++++++++++
>>   mm/memfd.c              |  9 ++++-----
>>   3 files changed, 31 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
>> index 45bf05a..3ddd69b 100644
>> --- a/include/linux/hugetlb.h
>> +++ b/include/linux/hugetlb.h
>> @@ -695,6 +695,9 @@ struct folio *alloc_hugetlb_folio(struct
>> vm_area_struct *vma,
>>   struct folio *alloc_hugetlb_folio_nodemask(struct hstate *h, int
>> preferred_nid,
>>   				nodemask_t *nmask, gfp_t gfp_mask,
>>   				bool allow_alloc_fallback);
>> +struct folio *alloc_hugetlb_folio_reserve(struct hstate *h, int preferred_nid,
>> +					  nodemask_t *nmask, gfp_t
>> gfp_mask);
>> +
>>   int hugetlb_add_to_page_cache(struct folio *folio, struct address_space
>> *mapping,
>>   			pgoff_t idx);
>>   void restore_reserve_on_error(struct hstate *h, struct vm_area_struct
>> *vma,
>> @@ -1062,6 +1065,13 @@ static inline struct folio
>> *alloc_hugetlb_folio(struct vm_area_struct *vma,
>>   }
>>
>>   static inline struct folio *
>> +alloc_hugetlb_folio_reserve(struct hstate *h, int preferred_nid,
>> +			    nodemask_t *nmask, gfp_t gfp_mask)
>> +{
>> +	return NULL;
>> +}
>> +
>> +static inline struct folio *
>>   alloc_hugetlb_folio_nodemask(struct hstate *h, int preferred_nid,
>>   			nodemask_t *nmask, gfp_t gfp_mask,
>>   			bool allow_alloc_fallback)
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index aaf508b..c2d44a1 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -2564,6 +2564,23 @@ struct folio
>> *alloc_buddy_hugetlb_folio_with_mpol(struct hstate *h,
>>   	return folio;
>>   }
>>
>> +struct folio *alloc_hugetlb_folio_reserve(struct hstate *h, int preferred_nid,
>> +		nodemask_t *nmask, gfp_t gfp_mask)
>> +{
>> +	struct folio *folio;
>> +
>> +	spin_lock_irq(&hugetlb_lock);
>> +	folio = dequeue_hugetlb_folio_nodemask(h, gfp_mask,
>> preferred_nid,
> I am assuming a check for available_huge_pages(h) before calling dequeue
> would be redundant as it would simply return NULL if no huge pages are
> available?

It would be wrong to check available_huge_pages() first, because it would return false
if free_huge_pages == resv_huge_pages, but at this point a page was already reserved
for us in resv_huge_pages, so we should always succeed.  The invariant here is
free_huge_pages >= resv_huge_pages. We simply do resv_huge_pages-- here, and do
free_huge_pages-- in the dequeue_hugetlb_folio_nodemask call stack.

- Steve

> Acked-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> 
> Thanks,
> Vivek
> 
>> +					       nmask);
>> +	if (folio) {
>> +		VM_BUG_ON(!h->resv_huge_pages);
>> +		h->resv_huge_pages--;
>> +	}
>> +
>> +	spin_unlock_irq(&hugetlb_lock);
>> +	return folio;
>> +}
>> +
>>   /* folio migration callback function */
>>   struct folio *alloc_hugetlb_folio_nodemask(struct hstate *h, int
>> preferred_nid,
>>   		nodemask_t *nmask, gfp_t gfp_mask, bool
>> allow_alloc_fallback)
>> diff --git a/mm/memfd.c b/mm/memfd.c
>> index e7b7c52..bfe0e71 100644
>> --- a/mm/memfd.c
>> +++ b/mm/memfd.c
>> @@ -82,11 +82,10 @@ struct folio *memfd_alloc_folio(struct file *memfd,
>> pgoff_t idx)
>>   		gfp_mask = htlb_alloc_mask(hstate_file(memfd));
>>   		gfp_mask &= ~(__GFP_HIGHMEM | __GFP_MOVABLE);
>>
>> -		folio = alloc_hugetlb_folio_nodemask(hstate_file(memfd),
>> -						     numa_node_id(),
>> -						     NULL,
>> -						     gfp_mask,
>> -						     false);
>> +		folio = alloc_hugetlb_folio_reserve(hstate_file(memfd),
>> +						    numa_node_id(),
>> +						    NULL,
>> +						    gfp_mask);
>>   		if (folio && folio_try_get(folio)) {
>>   			err = hugetlb_add_to_page_cache(folio,
>>   							memfd->f_mapping,
>> --
>> 1.8.3.1
>