From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 50C6AC433DB for ; Thu, 7 Jan 2021 03:09:19 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id C840E2229C for ; Thu, 7 Jan 2021 03:09:18 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C840E2229C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=bytedance.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 653488D011C; Wed, 6 Jan 2021 22:09:18 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 602DF8D0090; Wed, 6 Jan 2021 22:09:18 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4CBD58D011C; Wed, 6 Jan 2021 22:09:18 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 35C1B8D0090 for ; Wed, 6 Jan 2021 22:09:18 -0500 (EST) Received: from smtpin11.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id E5ED5363D for ; Thu, 7 Jan 2021 03:09:17 +0000 (UTC) X-FDA: 77677497954.11.grape65_40106cb274e6 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin11.hostedemail.com (Postfix) with ESMTP id C419F180F8B80 for ; Thu, 7 Jan 2021 03:09:17 +0000 (UTC) X-HE-Tag: grape65_40106cb274e6 X-Filterd-Recvd-Size: 9966 Received: from mail-pf1-f175.google.com (mail-pf1-f175.google.com [209.85.210.175]) by imf50.hostedemail.com (Postfix) with ESMTP for ; Thu, 7 Jan 2021 03:09:17 +0000 (UTC) Received: by mail-pf1-f175.google.com with SMTP id c79so2987813pfc.2 for ; Wed, 06 Jan 2021 19:09:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=rD99powxPx4r6hhXAyIjhyhZZ+nmITFUObXAUERyXlY=; b=Bk0JxjJdEDE7/4PWIkF+SGcHF7jdX3JFoEDoPNVRcrx1l0ik/XZPTwZY9on/Bz/2+r AOeVwfegqK8kvYzQRquZZe6uyhubH8HhFAprysqoSzxMG4ytax4SPP8dxwYkZRqf8nqA L13NyEmJqefaoC8mtcfnCzNpXW76Ph0s+XIvmTYu/0SlRFH234ARNMKxo/BupgycTg/n mG1muXK1yw4VDq3vT9v0MrQ8gJmw6lrxiQyJZSCFoLkTJLBrOrvTh7x5M2n/LKLhQKfO USYaGdJcsttfgD3LSq7wuXwZyRyeZmgCgk7UFbeLlOg30WrH3eWtdJIZ8BoSa20R9J7a yVJg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=rD99powxPx4r6hhXAyIjhyhZZ+nmITFUObXAUERyXlY=; b=n7gjzaIF22QPapeucDkGH+vEMUixMonzCrWSZ+zuTy3kd6jmmXelRNzsw0ByKl+zqK 5ruBilsdAr79nyDOv8Y7NAqUJ3iSfV5VvXPbGTEjcPPSBS0wK6a3z8gt/RliK5aDq+YV BLvphvUZYuIpb76R3kwvpi0YPopCKKJ8jll6s5qhkkkmwbYTkeBamEuhC7cPRS2MwanQ CKWVG5I0COwQtyW72ovlxZwZl/NiUnPHwySoxpXVC3Gp3shpvq6eSjvquDuXWIVu4MJQ IkD9v7dq94TyhZ1Lqdrs/WTzBgixNFRYK80hpvbuHL5G7NAhpuBUJvY5aWcAw1bArtWO tr1A== X-Gm-Message-State: AOAM530a6+EVzUtGmtG3BTOQ4MvFOiw7H+5bC/DPpaFMOZkt735iISFC fQcINl7hOirUyw2mXw0VnmQAANU3UkZTs3O9gAycEg== X-Google-Smtp-Source: ABdhPJwagtK5gxq7lflxscL2LOMEj3UGq3+cfm1TaSDiu/9I2ESCypact/j4hkxrjaxptrDHS+7UixupbbH4KrfGsCA= X-Received: by 2002:aa7:8701:0:b029:19e:561:d476 with SMTP id b1-20020aa787010000b029019e0561d476mr7080431pfo.2.1609988955884; Wed, 06 Jan 2021 19:09:15 -0800 (PST) MIME-Version: 1.0 References: <20210106084739.63318-1-songmuchun@bytedance.com> <20210106084739.63318-4-songmuchun@bytedance.com> <20210106165632.GT13207@dhcp22.suse.cz> In-Reply-To: From: Muchun Song Date: Thu, 7 Jan 2021 11:08:37 +0800 Message-ID: Subject: Re: [External] Re: [PATCH v2 3/6] mm: hugetlb: fix a race between freeing and dissolving the page To: Mike Kravetz Cc: Michal Hocko , Andrew Morton , Naoya Horiguchi , Andi Kleen , Linux Memory Management List , LKML Content-Type: text/plain; charset="UTF-8" X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, Jan 7, 2021 at 5:00 AM 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. > > 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. > > > > >> Fixes: c8721bbbdd36 ("mm: memory-hotplug: enable memory hotplug to handle hugepage") > >> Signed-off-by: Muchun Song > >> --- > >> mm/hugetlb.c | 38 ++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 38 insertions(+) > >> > >> diff --git a/mm/hugetlb.c b/mm/hugetlb.c > >> index 4741d60f8955..8ff138c17129 100644 > >> --- a/mm/hugetlb.c > >> +++ b/mm/hugetlb.c > >> @@ -79,6 +79,21 @@ DEFINE_SPINLOCK(hugetlb_lock); > >> static int num_fault_mutexes; > >> struct mutex *hugetlb_fault_mutex_table ____cacheline_aligned_in_smp; > >> > >> +static inline bool PageHugeFreed(struct page *head) > >> +{ > >> + return (unsigned long)head[3].mapping == -1U; > >> +} > >> + > >> +static inline void SetPageHugeFreed(struct page *head) > >> +{ > >> + head[3].mapping = (void *)-1U; > >> +} > >> + > >> +static inline void ClearPageHugeFreed(struct page *head) > >> +{ > >> + head[3].mapping = NULL; > >> +} > >> + > >> /* Forward declaration */ > >> static int hugetlb_acct_memory(struct hstate *h, long delta); > >> > >> @@ -1028,6 +1043,7 @@ static void enqueue_huge_page(struct hstate *h, struct page *page) > >> list_move(&page->lru, &h->hugepage_freelists[nid]); > >> h->free_huge_pages++; > >> h->free_huge_pages_node[nid]++; > >> + SetPageHugeFreed(page); > >> } > >> > >> static struct page *dequeue_huge_page_node_exact(struct hstate *h, int nid) > >> @@ -1044,6 +1060,7 @@ static struct page *dequeue_huge_page_node_exact(struct hstate *h, int nid) > >> > >> list_move(&page->lru, &h->hugepage_activelist); > >> set_page_refcounted(page); > >> + ClearPageHugeFreed(page); > >> h->free_huge_pages--; > >> h->free_huge_pages_node[nid]--; > >> return page; > >> @@ -1291,6 +1308,17 @@ static inline void destroy_compound_gigantic_page(struct page *page, > >> unsigned int order) { } > >> #endif > >> > >> +/* > >> + * Because we reuse the mapping field of some tail page structs, we should > >> + * reset those mapping to initial value before @head is freed to the buddy > >> + * allocator. The invalid value will be checked in the free_tail_pages_check(). > >> + */ > > When I suggested using head[3].mapping for this state, I was not aware of > this requirement. My suggestion was only following the convention used in > PageHugeTemporary. I would not have made the suggestion if I had realized > this was required. Sorry. Yeah, PageHugeTemporary is lucky. free_tail_pages_check() not check head[2].mapping. I will revert to the previous version (using head[3].private). Thanks. > -- > Mike Kravetz > > >> +static inline void reset_tail_page_mapping(struct hstate *h, struct page *head) > >> +{ > >> + if (!hstate_is_gigantic(h)) > >> + head[3].mapping = TAIL_MAPPING; > >> +} > >> + > >> static void update_and_free_page(struct hstate *h, struct page *page) > >> { > >> int i; > >> @@ -1298,6 +1326,7 @@ static void update_and_free_page(struct hstate *h, struct page *page) > >> if (hstate_is_gigantic(h) && !gigantic_page_runtime_supported()) > >> return; > >> > >> + reset_tail_page_mapping(h, page); > >> h->nr_huge_pages--; > >> h->nr_huge_pages_node[page_to_nid(page)]--; > >> for (i = 0; i < pages_per_huge_page(h); i++) { > >> @@ -1504,6 +1533,7 @@ static void prep_new_huge_page(struct hstate *h, struct page *page, int nid) > >> spin_lock(&hugetlb_lock); > >> h->nr_huge_pages++; > >> h->nr_huge_pages_node[nid]++; > >> + ClearPageHugeFreed(page); > >> spin_unlock(&hugetlb_lock); > >> } > >> > >> @@ -1770,6 +1800,14 @@ int dissolve_free_huge_page(struct page *page) > >> int nid = page_to_nid(head); > >> if (h->free_huge_pages - h->resv_huge_pages == 0) > >> goto out; > >> + > >> + /* > >> + * We should make sure that the page is already on the free list > >> + * when it is dissolved. > >> + */ > >> + if (unlikely(!PageHugeFreed(head))) > >> + goto out; > >> + > >> /* > >> * Move PageHWPoison flag from head page to the raw error page, > >> * which makes any subpages rather than the error page reusable. > >> -- > >> 2.11.0 > >