lkml.org 
[lkml]   [2021]   [Jan]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 3/6] mm: hugetlb: fix a race between freeing and dissolving the page
    On Wed 06-01-21 12:58:29, Mike Kravetz wrote:
    > On 1/6/21 8:56 AM, Michal Hocko wrote:
    > > On Wed 06-01-21 16:47:36, Muchun Song wrote:
    > >> 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.
    > >
    > >> 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

    \
     
     \ /
      Last update: 2021-01-07 09:42    [W:4.312 / U:0.164 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site