lkml.org 
[lkml]   [2021]   [Jan]   [8]   [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 Thu 07-01-21 16:52:19, Mike Kravetz wrote:
    > On 1/7/21 12:40 AM, Michal Hocko wrote:
    > > 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?
    >
    > The reservation code is so fragile, I always get nervous when making
    > any changes. However, the straight forward patch below passes some
    > simple testing. The only difference I can see is that global counts
    > are adjusted before sub-pool counts. This should not be an issue as
    > global and sub-pool counts are adjusted independently (not under the
    > same lock). Allocation code checks sub-pool counts before global
    > counts. So, there is a SMALL potential that a racing allocation which
    > previously succeeded would now fail. I do not think this is an issue
    > in practice.
    >
    > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
    > index 3b38ea958e95..658593840212 100644
    > --- a/mm/hugetlb.c
    > +++ b/mm/hugetlb.c
    > @@ -1395,6 +1395,11 @@ static void __free_huge_page(struct page *page)
    > (struct hugepage_subpool *)page_private(page);
    > bool restore_reserve;
    >
    > + spin_lock(&hugetlb_lock);
    > + /* check for race with dissolve_free_huge_page/update_and_free_page */
    > + if (!PageHuge(page))
    > + return;
    > +

    This really wants to unlock the lock, right? But this is indeed what
    I've had in mind.
    --
    Michal Hocko
    SUSE Labs

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