Re: [PATCH v2 5/9] mm/hugetlb: convert isolate_or_dissolve_huge_page to folios

[Sidhartha Kumar <sidhartha.kumar@oracle.com>]

On 6/12/23 10:41 AM, Matthew Wilcox wrote:
> On Tue, Nov 01, 2022 at 03:30:55PM -0700, Sidhartha Kumar wrote:
>> +++ b/mm/hugetlb.c
>> @@ -2815,7 +2815,7 @@ static int alloc_and_dissolve_huge_page(struct hstate *h, struct page *old_page,
>>   int isolate_or_dissolve_huge_page(struct page *page, struct list_head *list)
>>   {
>>   	struct hstate *h;
>> -	struct page *head;
>> +	struct folio *folio = page_folio(page);
> 
> Is this safe?  I was reviewing a different patch today, and I spotted
> this.  With THP, we can relatively easily hit this case:
> 
> struct page points to a page with pfn 0x40305, in a folio of order 2.
> We call page_folio() on it and the resulting pointer is for the folio
> with pfn 0x40304.
> If we don't have our own refcount (or some other protection ...) against
> freeing, the folio can now be freed and reallocated.  Say it's now part
> of an order-3 folio.
> Our 'folio' pointer is now actually a pointer to a tail page, and we
> have various assertions that a folio pointer doesn't point to a tail
> page, so they trigger.
> 
> It seems to me that this ...
> 
>          /*
>           * The page might have been dissolved from under our feet, so make sure
>           * to carefully check the state under the lock.
>           * Return success when racing as if we dissolved the page ourselves.
>           */
>          spin_lock_irq(&hugetlb_lock);
>          if (folio_test_hugetlb(folio)) {
>                  h = folio_hstate(folio);
>          } else {
>                  spin_unlock_irq(&hugetlb_lock);
>                  return 0;
>          }
> 
> implies that we don't have our own reference on the folio, so we might
> find a situation where the folio pointer we have is no longer a folio
> pointer.
> 

If the folio became free and reallocated would this be considered a 
success? If the folio is no longer a hugetlb folio, 
isolate_or_dissolve_huge_page() returns as if it dissolved the page itself.

Later in the call stack, within alloc_and_dissolve_hugetlb_folio() there is

	if (!folio_test_hugetlb(old_folio)) {
		/*
		 * Freed from under us. Drop new_folio too.
		 */
		goto free_new;
	}

which would imply it is safe for the old_folio to have been dropped/freed.


> Maybe the page_folio() call should be moved inside the hugetlb_lock
> protection?  Is that enough?  I don't know enough about how hugetlb
> pages are split, freed & allocated to know what's going on.
> But then we _drop_ the lock, and keep referring to ...
> 
>> @@ -2841,10 +2840,10 @@ int isolate_or_dissolve_huge_page(struct page *page, struct list_head *list)
>>   	if (hstate_is_gigantic(h))
>>   		return -ENOMEM;
>>   
>> -	if (page_count(head) && !isolate_hugetlb(head, list))
>> +	if (folio_ref_count(folio) && !isolate_hugetlb(&folio->page, list))
>>   		ret = 0;
>> -	else if (!page_count(head))
>> -		ret = alloc_and_dissolve_huge_page(h, head, list);
>> +	else if (!folio_ref_count(folio))
>> +		ret = alloc_and_dissolve_huge_page(h, &folio->page, list);
> 
> And I fall back to saying "I don't know enough to know if this is safe".