On Wed 06-01-21 12:58:29, Mike Kravetz wrote:

On 1/6/21 8:56 AM, Michal Hocko wrote:

quoted

On Wed 06-01-21 16:47:36, Muchun Song wrote:

quoted

There is a race condition between __free_huge_page()
and dissolve_free_huge_page().

CPU0:                         CPU1:

// page_count(page) == 1
put_page(page)
  __free_huge_page(page)
                              dissolve_free_huge_page(page)
                                spin_lock(&hugetlb_lock)
                                // PageHuge(page) && !page_count(page)
                                update_and_free_page(page)
                                // page is freed to the buddy
                                spin_unlock(&hugetlb_lock)
    spin_lock(&hugetlb_lock)
    clear_page_huge_active(page)
    enqueue_huge_page(page)
    // It is wrong, the page is already freed
    spin_unlock(&hugetlb_lock)

The race windows is between put_page() and spin_lock() which
is in the __free_huge_page().

The race window reall is between put_page and dissolve_free_huge_page.
And the result is that the put_page path would clobber an unrelated page
(either free or already reused page) which is quite serious.
Fortunatelly pages are dissolved very rarely. I believe that user would
require to be privileged to hit this by intention.

quoted

We should make sure that the page is already on the free list
when it is dissolved.

Another option would be to check for PageHuge in __free_huge_page. Have
you considered that rather than add yet another state? The scope of the
spinlock would have to be extended. If that sounds more tricky then can
we check the page->lru in the dissolve path? If the page is still
PageHuge and reference count 0 then there shouldn't be many options
where it can be queued, right?

The tricky part with expanding lock scope will be the potential call to
hugepage_subpool_put_pages as it may also try to acquire the hugetlb_lock.

Can we rearrange the code and move hugepage_subpool_put_pages after all
this is done? Or is there any strong reason for the particular ordering?

I am not sure what you mean by 'check the page->lru'?  If we knew the page
was on the free list, then we could dissolve.  But, I do not think there
is an easy way to determine that from page->lru.  A hugetlb page is either
going to be on the active list or free list.

Can it be on the active list with ref count = 0?
-- 
Michal Hocko
SUSE Labs