From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A2B9E34A773 for ; Mon, 27 Jul 2026 15:19:53 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=90.155.50.34 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785165596; cv=none; b=awU43qW8Yu/PsI8oqQOVpL6V9ChQd009I6TIO/khAarr34qvhWT4ACiM6gDiEYIQndXOAZ1Kq/TzPkA2bLvIG4RAdPrPTXEeg/sJPUHoVdjVv+DX9cqakauq0YIIvteq7bLSslGUzFYyysVyccT9x8xr6lMkVYx8wS+w7TniDH0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785165596; c=relaxed/simple; bh=WsEr1wwwwCva7mpr+Saa31SuWxenbzqVqHarZ/gD754=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=PKwM6To8QUTNJvTHvtM+WLYETtZCUsQnMrriDQrxDQrJRCNReuQe4P1al9pg/vDIELvdHIxp9bakN+x5soxWtyNMw4NbZoHfjlEB6QZLwevAlZXcdt0fDcIPkkHHJvV9LnoGCu0A6sN+MOMNnUYv4I+ZxlwzjqUr3oVFBL3xy5A= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=infradead.org; spf=pass smtp.mailfrom=infradead.org; dkim=pass (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b=DRBgB5eu; arc=none smtp.client-ip=90.155.50.34 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=infradead.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=infradead.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b="DRBgB5eu" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=kvFaYGmnPcUYTMTMoWdVLVVzjSaeldohJlXUNlxwbWo=; b=DRBgB5euulFhy0xU2HuVK0AR+J KyR8Ny4HEIa/BGaV3ArU4UbqXlOZheUfRnHdiZf5UnIPAy69Mnq2tlLNNmAyInO6StaHKy/fM9Sjl LPEkBN9DhmOevrhmbykAi9PKcuMt9QtuKBbUX4dUd9j8oN6jnQe+TpD7NuTsWWn08bWRHdtmeR5s3 bJVr6Hv1AhEmxqgPScSi1cmbM8wJod0aNSeCRr49Dj1hoZG3JG5zaWmT9cKCfZvqNbfZCmAe8ZMc4 VlWpmshZ+uot4zz37YUO1BwOw1Vsi14A+VYqXNQGBQ/zIvrLJdhOlgCa/ogYDejHE9BDDOm9K0WGp SkU57kBQ==; Received: from willy by casper.infradead.org with local (Exim 4.99.1 #2 (Red Hat Linux)) id 1woN7F-0000000Avqo-1yf6; Mon, 27 Jul 2026 15:19:41 +0000 Date: Mon, 27 Jul 2026 16:19:41 +0100 From: Matthew Wilcox To: Andrew Morton , Jane Chu , linux-mm@kvack.org Cc: Muchun Song , Oscar Salvador , David Hildenbrand , Miaohe Lin , Naoya Horiguchi , Jan Kara , linux-fsdevel@vger.kernel.org, Christian Brauner , Jiaqi Yan Subject: Re: [PATCH v5 05/12] hugetlb: Use the has_hwpoisoned flag Message-ID: References: <20260725160042.1557264-1-willy@infradead.org> <20260725160042.1557264-6-willy@infradead.org> Precedence: bulk X-Mailing-List: linux-fsdevel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260725160042.1557264-6-willy@infradead.org> Sashiko: > --- Patch [5]: [PATCH v5 05/12] hugetlb: Use the has_hwpoisoned flag --- > commit cca87d250a7a597186556cb9d897807edae0b82f > Author: Matthew Wilcox (Oracle) > > hugetlb: Use the has_hwpoisoned flag > > This commit converts hugetlb to use the has_hwpoisoned flag to match > other large folios. It introduces folio_test_huge_poison() for lockless > checks to close a gap where a poisoned hugetlb folio could be observed > without the hwpoison bit set during teardown. > > > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h > > --- a/include/linux/page-flags.h > > +++ b/include/linux/page-flags.h > [ ... ] > > @@ -1041,8 +1046,29 @@ PAGE_TYPE_OPS(Slab, slab, slab) > > > > #ifdef CONFIG_HUGETLB_PAGE > > FOLIO_TYPE_OPS(hugetlb, hugetlb) > > + > > +#ifdef CONFIG_MEMORY_FAILURE > > +static inline bool folio_test_huge_poison(const struct folio *folio) > > +{ > > + return (READ_ONCE(folio->page.page_type) >> 23) == > > + ((PGTY_hugetlb << 1) | 1); > > +} > > This is a pre-existing issue, but does this logic fully close the race > window? > > The folio_test_huge_poison() function assumes the PGTY_hugetlb bits > remain intact during teardown. However, __ClearPagehugetlb() > unconditionally sets the entire page_type to UINT_MAX. > > When __ClearPagehugetlb() is called, folio_test_huge_poison() will evaluate > to false at the exact same moment folio_test_hugetlb() evaluates to false. > Would this allow a concurrent is_page_hwpoison() check to miss the poison > state? Um, no? Here's how is_page_hwpoison() looks: static inline bool is_page_hwpoison(const struct page *page) { const struct folio *folio; if (PageHWPoison(page)) return true; folio = page_folio(page); if (folio_test_huge_poison(folio)) return true; /* In case we raced with hugetlb transferring flags */ return PageHWPoison(page); } We're relying on ordering; if we see a false folio_test_huge_poison(), we recheck that the flag hasn't already been moved to the page. Since we always move the poison to the pages before clearing the page type, I don't see how we can miss anything. > > +static inline void folio_set_huge_poison(struct folio *folio) > > +{ > > + folio->page.page_type |= (1 << 23); > > +} > > + > > +static inline void folio_clear_huge_poison(struct folio *folio) > > +{ > > + folio->page.page_type &= ~(1 << 23); > > +} > > Can this cause a data race on the page_type field? > > In folio_clear_huge_poison(), the bitwise operation modifies the page_type > field non-atomically. If unpoison_memory() calls this on a free hugetlb > folio while a concurrent sysfs pool shrink calls __ClearPagehugetlb() > under the hugetlb_lock, the UINT_MAX value could be overwritten with > 0xFF7FFFFF. > > Since unpoison_memory() relies on mf_mutex rather than hugetlb_lock, > would this corrupted page_type trigger a VM_BUG_ON_PAGE in the buddy > allocator when the page reaches free_pages_prepare()? Hmm ... if we just move the hugetlb_clear_poison() call before the call to folio_put(), that'll take care of this race, right? Oh, but there's two calls to folio_free_raw_hwp(), and the other one is when (ugh), we don't have a reference to the folio. So I think we wrap the whole thing in hugetlb_lock to prevent this? ghp = get_hwpoison_page(p, MF_UNPOISON); if (!ghp) { spin_lock_irq(&hugetlb_lock); if (folio_test_hugetlb(folio)) { huge = true; count = folio_free_raw_hwp(folio, false); if (count == 0) goto unlock_mutex; ret = hugetlb_clear_poison(folio); } else { ret = TestClearPageHWPoison(p) ? 0 : -EBUSY; } spin_unlock_irq(&hugetlb_lock); (this also includes the fix to the problem discussed below) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > --- a/mm/hugetlb.c > > +++ b/mm/hugetlb.c > [ ... ] > > @@ -1498,10 +1498,10 @@ static void __update_and_free_hugetlb_folio(struct hstate *h, > > } > > > > /* > > - * Move PageHWPoison flag from head page to the raw error pages, > > - * which makes any healthy subpages reusable. > > + * Move HWPoison flag to each error page > > + * which makes any healthy pages reusable. > > */ > > - if (unlikely(folio_test_hwpoison(folio))) > > + if (unlikely(folio_test_has_hwpoisoned(folio))) > > folio_clear_hugetlb_hwpoison(folio); > > > > VM_BUG_ON_FOLIO(folio_ref_count(folio), folio); > > This is a pre-existing issue, but is there a risk of a double free in the > hwpoison list teardown? > > Both __update_and_free_hugetlb_folio() (via folio_clear_hugetlb_hwpoison()) > and unpoison_memory() can invoke __folio_free_raw_hwp() on the same folio > concurrently. > > Looking at __folio_free_raw_hwp() in mm/memory-failure.c, it reads > folio->hugetlb_hwpoison and sets it to NULL without any synchronization > locks. If both threads read the pointer before either sets it to NULL, > wouldn't they concurrently iterate the linked list and call kfree() on the > same hwp_page elements? I think that's right, but I also think it goes away later in this series when hwp_page_lock serialises the calls to __folio_free_raw_hwp(). > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > > --- a/mm/memory-failure.c > > +++ b/mm/memory-failure.c > [ ... ] > > @@ -2730,8 +2753,8 @@ int unpoison_memory(unsigned long pfn) > > count = folio_free_raw_hwp(folio, false); > > if (count == 0) > > goto unlock_mutex; > > + ret = hugetlb_clear_poison(folio); > > } > > - ret = folio_test_clear_hwpoison(folio) ? 0 : -EBUSY; > > } else if (ghp < 0) { > > if (ghp == -EHWPOISON) { > > ret = put_page_back_buddy(p) ? 0 : -EBUSY; > > @@ -2740,17 +2763,18 @@ int unpoison_memory(unsigned long pfn) > > unpoison_pr_info("%#lx: failed to grab page\n", > > pfn, &unpoison_rs); > > } > > Does this logic change drop the ability to unpoison free non-hugetlb pages? > > The refactoring of the if (!ghp) block removed the fallback logic that > handled non-hugetlb pages. If get_hwpoison_page() returns 0 for a free > buddy page, execution enters the if (!ghp) block. > > Because folio_test_hugetlb(folio) is false, it does nothing, drops through > the entire if/else chain, and returns -EBUSY. Could this leave free buddy > pages permanently poisoned? Oh, I see. I hadn't realised that case was also being handled here. I'll rework it.