On Wed, Apr 21, 2021 at 11:03:24AM -0700, Mike Kravetz wrote: > On 4/21/21 1:33 AM, HORIGUCHI NAOYA(堀口 直也) wrote: > > On Wed, Apr 21, 2021 at 10:03:34AM +0200, Michal Hocko wrote: > >> [Cc Naoya] > >> > >> On Wed 21-04-21 14:02:59, Muchun Song wrote: > >>> The possible bad scenario: > >>> > >>> CPU0: CPU1: > >>> > >>> gather_surplus_pages() > >>> page = alloc_surplus_huge_page() > >>> memory_failure_hugetlb() > >>> get_hwpoison_page(page) > >>> __get_hwpoison_page(page) > >>> get_page_unless_zero(page) > >>> zero = put_page_testzero(page) > >>> VM_BUG_ON_PAGE(!zero, page) > >>> enqueue_huge_page(h, page) > >>> put_page(page) > >>> > >>> The refcount can possibly be increased by memory-failure or soft_offline > >>> handlers, we can trigger VM_BUG_ON_PAGE and wrongly add the page to the > >>> hugetlb pool list. > >> > >> The hwpoison side of this looks really suspicious to me. It shouldn't > >> really touch the reference count of hugetlb pages without being very > >> careful (and having hugetlb_lock held). > > > > I have the same feeling, there is a window where a hugepage is refcounted > > during converting from buddy free pages into free hugepage, so refcount > > alone is not enough to prevent the race. hugetlb_lock is retaken after > > alloc_surplus_huge_page returns, so simply holding hugetlb_lock in > > get_hwpoison_page() seems not work. Is there any status bit to show that a > > hugepage is just being initialized (not in free hugepage pool or in use)? > > > > It seems we can also race with the code that makes a compound page a > hugetlb page. The memory failure code could be called after allocating > pages from buddy and before setting compound page DTOR. So, the memory > handling code will process it as a compound page. Yes, so get_hwpoison_page() has to call get_page_unless_zero() only when memory_failure() can surely handle the error. > > Just thinking that this may not be limited to the hugetlb specific memory > failure handling? Currently hugetlb page is the only type of compound page supported by memory failure. But I agree with you that other types of compound pages have the same race window, and judging only with get_page_unless_zero() is dangerous. So I think that __get_hwpoison_page() should have the following structure: if (PageCompound) { if (PageHuge) { if (PageHugeFreed || PageHugeActive) { if (get_page_unless_zero) return 0; // path for in-use hugetlb page else return 1; // path for free hugetlb page } else { return -EBUSY; // any transient hugetlb page } } else { ... // any other compound page (like thp, slab, ...) } } else { ... // any non-compound page } Thanks, Naoya Horiguchi