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.
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".
