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 B07BBA55 for ; Tue, 21 Jan 2025 05:00:21 +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=1737435625; cv=none; b=ZwWQpyVMbo1tVqnKAXy4QLkiE9qH7I9RXpsBFqtZKTcagFePuiDLEnbj2Ia6/mxCp72Hrxr5VHOngP3HTxyCe+0bW/TF73gcXcl+HGwf10xL7bfpKloKzzxS+SQB2Rxf6fV5I827fur0ejIxEtcDJIDzxGaCTQMX7xZcwMMuIJA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737435625; c=relaxed/simple; bh=+uUWivQL8brOQyWcZPXVb8otoGvvB6qeOvpGIFlyWIM=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=GgPtIj+6ii5SfN/GrXoq1G/G/yWIQk9l27ARl0hxzVA/A6Hb4AKyc/r/Tkcw44h1hfD1OeRRKdLa3NWTidnPSam7DSzorxnD1kLoNAvshwkk2BtSWyp/+zcUelOJRBACMHtc1bCOlTqCWDSAbtUJBnMojZ8ceM06Ntm4hmvRS6w= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=infradead.org; spf=none smtp.mailfrom=infradead.org; dkim=pass (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b=WJofv2U3; arc=none smtp.client-ip=90.155.50.34 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=infradead.org Authentication-Results: smtp.subspace.kernel.org; spf=none 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="WJofv2U3" 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=sVJNRYcC45C+fk8/emdWmRDSfn/hmkyWe5KKx8RKg8o=; b=WJofv2U3TYkJUlgOSAaerZKUEA 19LpGlT3PdmhsUuYxBq/baxqATuA6rAH+NfFVedJ9LVxz7sSKMkDUxEjJTg2JGgkpgrL+tN+VDR9R e0KPx4k/XaPNAbeR4/Np5libYaqhCGjdNdnkITTx5GG8oFJT4nLhSsf3Pael3VR0KoP5BcKM2wAo8 MQETD520QroOHAhorBJH3GmqaDJa0b+xpqzjZQvyYdZ+km/45foLUtHPGhhelmts59t0y5J8697z9 tuQXM4MeGLUCiaDey0OsY3ijloPH0ws/yvrsxIbEQTkjkc3QCtaH+OZPcxO/gGOO0jppKz/xGUQ/5 0001tn7Q==; Received: from willy by casper.infradead.org with local (Exim 4.98 #2 (Red Hat Linux)) id 1ta6N0-0000000FpWy-2EN8; Tue, 21 Jan 2025 05:00:10 +0000 Date: Tue, 21 Jan 2025 05:00:10 +0000 From: Matthew Wilcox To: Jane Chu Cc: akpm@linux-foundation.org, linmiaohe@huawei.com, kirill.shutemov@linux.intel.com, hughd@google.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mm: make page_mapped_in_vma() hugetlb walk aware Message-ID: References: <20250121041849.3393237-1-jane.chu@oracle.com> Precedence: bulk X-Mailing-List: linux-kernel@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: <20250121041849.3393237-1-jane.chu@oracle.com> On Mon, Jan 20, 2025 at 09:18:49PM -0700, Jane Chu wrote: > When a process consumes a UE in a page, the memory failure handler > attempts to collect information for a potential SIGBUS. > If the page is an anonymous page, page_mapped_in_vma(page, vma) is > invoked in order to > 1. retrieve the vaddr from the process' address space, > 2. verify that the vaddr is indeed mapped to the poisoned page, > where 'page' is the precise small page with UE. > > It's been observed that when injecting poison to a non-head subpage > of an anonymous hugetlb page, no SIGBUS show up; while injecting to > the head page produces a SIGBUS. The casue is that, though hugetlb_walk() > returns a valid pmd entry (on x86), but check_pte() detects mismatch > between the head page per the pmd and the input subpage. Thus the vaddr > is considered not mapped to the subpage and the process is not collected > for SIGBUS purpose. This is the calling stack > collect_procs_anon > page_mapped_in_vma > page_vma_mapped_walk > hugetlb_walk > huge_pte_lock > check_pte > > It seems that the most obvious place to fix the issue is by making > page_mapped_in_vma() hugetlb walk aware. The precise subpage in the > input is useful in providing PAGE_SIZE granularity vaddr. I don't like this solution because it adds yet another special case for hugetlb. If we don't split a PMD-mapped THP, we'd have the same problem, right? check_pte() would succeed if we set pvmw->pfn to folio_pfn() and pvmw->nr_pages to folio_nr_pages(), right? I just don't know what else might be affected by that. I like one of these two options: @@ -206,6 +206,7 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw) pvmw->pte = hugetlb_walk(vma, pvmw->address, size); if (!pvmw->pte) return false; + pvmw->pte += pvmw->address & (size - PAGE_SIZE); pvmw->ptl = huge_pte_lock(hstate, mm, pvmw->pte); if (!check_pte(pvmw)) (that needs a bit of tidying up; you can't just do that, but I think you get the basic idea -- correct the pte to point to the precise page instead of the hugetlb pfn) The option I really prefer is much more work but matches our preferred direction of getting rid of hugetlb specific code. Something like this: @@ -192,27 +192,6 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw) if (pvmw->pmd && !pvmw->pte) return not_found(pvmw); - if (unlikely(is_vm_hugetlb_page(vma))) { - struct hstate *hstate = hstate_vma(vma); - unsigned long size = huge_page_size(hstate); - /* The only possible mapping was handled on last iteration */ [...] - pvmw->ptl = huge_pte_lock(hstate, mm, pvmw->pte); - if (!check_pte(pvmw)) - return not_found(pvmw); - return true; - } - end = vma_address_end(pvmw); if (pvmw->pte) goto next_pte; @@ -229,7 +208,19 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw continue; } pud = pud_offset(p4d, pvmw->address); - if (!pud_present(*pud)) { + pude = *pud; + if (pud_trans_huge(pude) || + (pud_present(pude) && pud_devmap(pude))) { + pvmw->ptl = pud_lock(mm, pvmw->pud); + ... + if (likely(pud_trans_huge(pude) || pud_devmap(pude))) { + if (pvmw->flags & PVMW_MIGRATION) + return not_found(pvmw); + if (!check_pud(pud_pfn(pude), pvmw)) + return not_found(pvmw); + return true; + } + } else if (!pud_present(pude)) { step_forward(pvmw, PUD_SIZE); continue; } ie get rid of all the hugetlb-specific code, and add support for the PUD level to the common code. You'd also need to write check_pud(). I'll understand if you don't want to do all the extra work. And thanks for tracking down this bug.