Re: [PATCH V1 4/5] mm/gup: fix memfd_pin_folios hugetlb page allocation

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

On 9/3/2024 9:06 PM, Kasireddy, Vivek wrote:
> Hi Steve,
> 
>> Subject: [PATCH V1 4/5] mm/gup: fix memfd_pin_folios hugetlb page
>> allocation
>>
>> When memfd_pin_folios -> memfd_alloc_folio creates a hugetlb page, the
>> index is wrong.  The subsequent call to filemap_get_folios_contig thus
>> cannot find it, and fails, and memfd_pin_folios loops forever.
>> To fix, adjust the index for the huge_page_order.
>>
>> memfd_alloc_folio also forgets to unlock the folio, so the next touch
>> of the page calls hugetlb_fault which blocks forever trying to take
>> the lock.  Unlock it.
> Where exactly is the lock taken from? I did a quick search but couldn't
> immediately figure out in which function is the lock taken, while allocating
> the folio.

memfd_alloc_folio -> hugetlb_add_to_page_cache -> __folio_set_locked

I forgot to add that detail to the commit message.

See for example hugetlbfs_fallocate which calls hugetlb_add_to_page_cache and
then calls folio_unlock before returning.

- Steve

>> Fixes: 89c1905d9c14 ("mm/gup: introduce memfd_pin_folios() for pinning
>> memfd folios")
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>   mm/memfd.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/memfd.c b/mm/memfd.c
>> index bfe0e71..bcb131d 100644
>> --- a/mm/memfd.c
>> +++ b/mm/memfd.c
>> @@ -79,10 +79,13 @@ struct folio *memfd_alloc_folio(struct file *memfd,
>> pgoff_t idx)
>>   		 * alloc from. Also, the folio will be pinned for an indefinite
>>   		 * amount of time, so it is not expected to be migrated away.
>>   		 */
>> -		gfp_mask = htlb_alloc_mask(hstate_file(memfd));
>> +		struct hstate *h = hstate_file(memfd);
>> +
>> +		gfp_mask = htlb_alloc_mask(h);
>>   		gfp_mask &= ~(__GFP_HIGHMEM | __GFP_MOVABLE);
>> +		idx >>= huge_page_order(h);
>>
>> -		folio = alloc_hugetlb_folio_reserve(hstate_file(memfd),
>> +		folio = alloc_hugetlb_folio_reserve(h,
>>   						    numa_node_id(),
>>   						    NULL,
>>   						    gfp_mask);
>> @@ -95,6 +98,7 @@ struct folio *memfd_alloc_folio(struct file *memfd,
>> pgoff_t idx)
>>   				free_huge_folio(folio);
>>   				return ERR_PTR(err);
>>   			}
>> +			folio_unlock(folio);
> Acked-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> 
> Thanks,
> Vivek
> 
>>   			return folio;
>>   		}
>>   		return ERR_PTR(-ENOMEM);
>> --
>> 1.8.3.1
>