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=-8.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 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 E52F0C34047 for ; Wed, 19 Feb 2020 12:21:48 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 7104221D56 for ; Wed, 19 Feb 2020 12:21:48 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7104221D56 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=huawei.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 206B26B000C; Wed, 19 Feb 2020 07:21:48 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 1DEE36B000D; Wed, 19 Feb 2020 07:21:48 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0CE626B000E; Wed, 19 Feb 2020 07:21:48 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0031.hostedemail.com [216.40.44.31]) by kanga.kvack.org (Postfix) with ESMTP id E9EE96B000C for ; Wed, 19 Feb 2020 07:21:47 -0500 (EST) Received: from smtpin09.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 98C21180AD815 for ; Wed, 19 Feb 2020 12:21:47 +0000 (UTC) X-FDA: 76506787854.09.duck58_83ea170cdaf29 X-HE-Tag: duck58_83ea170cdaf29 X-Filterd-Recvd-Size: 11184 Received: from huawei.com (szxga06-in.huawei.com [45.249.212.32]) by imf04.hostedemail.com (Postfix) with ESMTP for ; Wed, 19 Feb 2020 12:21:46 +0000 (UTC) Received: from DGGEMS408-HUB.china.huawei.com (unknown [172.30.72.60]) by Forcepoint Email with ESMTP id A1764FAFA392918E439C; Wed, 19 Feb 2020 20:21:37 +0800 (CST) Received: from [127.0.0.1] (10.177.246.209) by DGGEMS408-HUB.china.huawei.com (10.3.19.208) with Microsoft SMTP Server id 14.3.439.0; Wed, 19 Feb 2020 20:21:27 +0800 Subject: Re: [PATCH] mm/hugetlb: avoid get wrong ptep caused by race To: Sean Christopherson CC: , , , , , , , References: <1582027825-112728-1-git-send-email-longpeng2@huawei.com> <20200218203717.GE28156@linux.intel.com> <20200219015836.GM28156@linux.intel.com> From: "Longpeng (Mike)" Message-ID: <6ccbde03-953c-c006-a07e-8146b84389d9@huawei.com> Date: Wed, 19 Feb 2020 20:21:26 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 MIME-Version: 1.0 In-Reply-To: <20200219015836.GM28156@linux.intel.com> Content-Type: text/plain; charset="utf-8" X-Originating-IP: [10.177.246.209] X-CFilter-Loop: Reflected Content-Transfer-Encoding: quoted-printable 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: =E5=9C=A8 2020/2/19 9:58, Sean Christopherson =E5=86=99=E9=81=93: > On Wed, Feb 19, 2020 at 09:39:59AM +0800, Longpeng (Mike) wrote: >> =E5=9C=A8 2020/2/19 4:37, Sean Christopherson =E5=86=99=E9=81=93: >>> On Tue, Feb 18, 2020 at 08:10:25PM +0800, Longpeng(Mike) wrote: >>>> Our machine encountered a panic after run for a long time and >>>> the calltrace is: >>> >>> What's the actual panic? Is it a BUG() in hugetlb_fault(), a bad poi= nter >>> dereference, etc...? >>> >> A bad pointer dereference. >> >> pgd -> pud -> user 1G hugepage >> huge_pte_offset() wants to return NULL or pud (point to the entry), bu= t it maybe >> return the a bad pointer of the user 1G hugepage. >> >>>> RIP: 0010:[] [] hugetlb_fault+0= x307/0xbe0 >>>> RSP: 0018:ffff9567fc27f808 EFLAGS: 00010286 >>>> RAX: e800c03ff1258d48 RBX: ffffd3bb003b69c0 RCX: e800c03ff1258d48 >>>> RDX: 17ff3fc00eda72b7 RSI: 00003ffffffff000 RDI: e800c03ff1258d48 >>>> RBP: ffff9567fc27f8c8 R08: e800c03ff1258d48 R09: 0000000000000080 >>>> R10: ffffaba0704c22a8 R11: 0000000000000001 R12: ffff95c87b4b60d8 >>>> R13: 00005fff00000000 R14: 0000000000000000 R15: ffff9567face8074 >>>> FS: 00007fe2d9ffb700(0000) GS:ffff956900e40000(0000) knlGS:00000000= 00000000 >>>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >>>> CR2: ffffd3bb003b69c0 CR3: 000000be67374000 CR4: 00000000003627e0 >>>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 >>>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 >>>> Call Trace: >>>> [] ? unlock_page+0x2b/0x30 >>>> [] ? hugetlb_fault+0x222/0xbe0 >>>> [] follow_hugetlb_page+0x175/0x540 >>>> [] ? cpumask_next_and+0x35/0x50 >>>> [] __get_user_pages+0x2a0/0x7e0 >>>> [] __get_user_pages_unlocked+0x15d/0x210 >>>> [] __gfn_to_pfn_memslot+0x3c5/0x460 [kvm] >>>> [] try_async_pf+0x6e/0x2a0 [kvm] >>>> [] tdp_page_fault+0x151/0x2d0 [kvm] >>>> [] ? vmx_vcpu_run+0x2ec/0xc80 [kvm_intel] >>>> [] ? vmx_vcpu_run+0x2f8/0xc80 [kvm_intel] >>>> [] kvm_mmu_page_fault+0x31/0x140 [kvm] >>>> [] handle_ept_violation+0x9e/0x170 [kvm_intel] >>>> [] vmx_handle_exit+0x2bc/0xc70 [kvm_intel] >>>> [] ? __vmx_complete_interrupts.part.73+0x80/0xd0 = [kvm_intel] >>>> [] ? vmx_vcpu_run+0x490/0xc80 [kvm_intel] >>>> [] vcpu_enter_guest+0x7be/0x13a0 [kvm] >>>> [] ? kvm_check_async_pf_completion+0x8e/0xb0 [kvm= ] >>>> [] kvm_arch_vcpu_ioctl_run+0x330/0x490 [kvm] >>>> [] kvm_vcpu_ioctl+0x309/0x6d0 [kvm] >>>> [] ? dequeue_signal+0x32/0x180 >>>> [] ? do_sigtimedwait+0xcd/0x230 >>>> [] do_vfs_ioctl+0x3f0/0x540 >>>> [] SyS_ioctl+0xa1/0xc0 >>>> [] system_call_fastpath+0x22/0x27 >>>> >>>> ( The kernel we used is older, but we think the latest kernel also h= as this >>>> bug after dig into this problem. ) >>>> >>>> For 1G hugepages, huge_pte_offset() wants to return NULL or pudp, bu= t it >>>> may return a wrong 'pmdp' if there is a race. Please look at the fol= lowing >>>> code snippet: >>>> ... >>>> pud =3D pud_offset(p4d, addr); >>>> if (sz !=3D PUD_SIZE && pud_none(*pud)) >>>> return NULL; >>>> /* hugepage or swap? */ >>>> if (pud_huge(*pud) || !pud_present(*pud)) >>>> return (pte_t *)pud; >>>> >>>> pmd =3D pmd_offset(pud, addr); >>>> if (sz !=3D PMD_SIZE && pmd_none(*pmd)) >>>> return NULL; >>>> /* hugepage or swap? */ >>>> if (pmd_huge(*pmd) || !pmd_present(*pmd)) >>>> return (pte_t *)pmd; >>>> ... >>>> >>>> The following sequence would trigger this bug: >>>> 1. CPU0: sz =3D PUD_SIZE and *pud =3D 0 , continue >>>> 1. CPU0: "pud_huge(*pud)" is false >>>> 2. CPU1: calling hugetlb_no_page and set *pud to xxxx8e7(PRESENT) >>>> 3. CPU0: "!pud_present(*pud)" is false, continue >>>> 4. CPU0: pmd =3D pmd_offset(pud, addr) and maybe return a wrong pmdp >>>> However, we want CPU0 to return NULL or pudp. >>>> >>>> We can avoid this race by read the pud only once. >>> >>> Are there any other options for avoiding the panic you hit? I ask be= cause >>> there are a variety of flows that use a very similar code pattern, e.= g. >>> lookup_address_in_pgd(), and using READ_ONCE() in huge_pte_offset() b= ut not >>> other flows could be confusing (or in my case, anxiety inducing[*]). = At >>> the least, adding a comment in huge_pte_offset() to explain the need = for >>> READ_ONCE() would be helpful. >>> >> I hope the hugetlb and mm maintainers could give some other options if= they >> approve this bug. >=20 > The race and the fix make sense. I assumed dereferencing garbage from = the > huge page was the issue, but I wasn't 100% that was the case, which is = why > I asked about alternative fixes. >=20 >> We change the code from >> if (pud_huge(*pud) || !pud_present(*pud)) >> to >> if (pud_huge(*pud) >> return (pte_t *)pud; >> busy loop for 500ms >> if (!pud_present(*pud)) >> return (pte_t *)pud; >> and the panic will be hit quickly. >> >> ARM64 has already use READ/WRITE_ONCE to access the pagetable, look at= this >> commit 20a004e7 (arm64: mm: Use READ_ONCE/WRITE_ONCE when accessing pa= ge tables). >> >> The root cause is: 'if (pud_huge(*pud) || !pud_present(*pud))' read en= try from >> pud twice and the *pud maybe change in a race, so if we only read the = pud once. >> I use READ_ONCE here is just for safe, to prevents the complier mischi= ef if >> possible. >=20 > FWIW, I'd be in favor of going the READ/WRITE_ONCE() route for x86, e.g= . > convert everything as a follow-up patch (or patches). I'm fairly confi= dent > that KVM's usage of lookup_address_in_mm() is safe, but I wouldn't exac= tly > bet my life on it. I'd much rather the failing scenario be that KVM us= es > a sub-optimal page size as opposed to exploding on a bad pointer. >=20 Um...our testcase starts 50 VMs with 2U4G(use 1G hugepage) and then do live-upgrade(private feature that just modify the qemu and libvirt) and live-migrate in turns for each one. However our live upgraded new QEMU wo= n't do touch_all_pages. Suppose we start a VM without touch_all_pages in QEMU, the VM's guest mem= ory is not mapped in the CR3 pagetable at the moment. When the 2 vcpus running, = they could access some pages belong to the same 1G-hugepage, both of them will= vmexit due to ept_violation and then call gup-->follow_hugetlb_page-->hugetlb_fa= ult, so the race may encounter, right? >> I'll add comments in v2. >> >>> [*] In kernel 5.6, KVM is moving to using lookup_address_in_pgd() (vi= a >>> lookup_address_in_mm()) to identify large page mappings. The fun= ction >>> itself is susceptible to such a race, but KVM only does the looku= p >>> after it has done gup() and also ensures any zapping of ptes will= cause >>> KVM to restart the faulting (guest) instruction or that the zap w= ill be >>> blocked until after KVM does the lookup, i.e. racing with a trans= ition >>> from !PRESENT -> PRESENT should be impossible (in theory). >>> >> This bug is from hugetlb core, we could trigger it in other usages eve= n if the >> latest KVM won't. >=20 > I was actually worried about the opposite, introducing a bug by moving = to > lookup_address_in_mm(). >=20 >>>> Signed-off-by: Longpeng(Mike) >>>> --- >>>> mm/hugetlb.c | 34 ++++++++++++++++++---------------- >>>> 1 file changed, 18 insertions(+), 16 deletions(-) >>>> >>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >>>> index dd8737a..3bde229 100644 >>>> --- a/mm/hugetlb.c >>>> +++ b/mm/hugetlb.c >>>> @@ -4908,31 +4908,33 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, >>>> pte_t *huge_pte_offset(struct mm_struct *mm, >>>> unsigned long addr, unsigned long sz) >>>> { >>>> - pgd_t *pgd; >>>> - p4d_t *p4d; >>>> - pud_t *pud; >>>> - pmd_t *pmd; >>>> + pgd_t *pgdp; >>>> + p4d_t *p4dp; >>>> + pud_t *pudp, pud; >>>> + pmd_t *pmdp, pmd; >>>> =20 >>>> - pgd =3D pgd_offset(mm, addr); >>>> - if (!pgd_present(*pgd)) >>>> + pgdp =3D pgd_offset(mm, addr); >>>> + if (!pgd_present(*pgdp)) >>>> return NULL; >>>> - p4d =3D p4d_offset(pgd, addr); >>>> - if (!p4d_present(*p4d)) >>>> + p4dp =3D p4d_offset(pgdp, addr); >>>> + if (!p4d_present(*p4dp)) >>>> return NULL; >>>> =20 >>>> - pud =3D pud_offset(p4d, addr); >>>> - if (sz !=3D PUD_SIZE && pud_none(*pud)) >>>> + pudp =3D pud_offset(p4dp, addr); >>>> + pud =3D READ_ONCE(*pudp); >>>> + if (sz !=3D PUD_SIZE && pud_none(pud)) >>>> return NULL; >>>> /* hugepage or swap? */ >>>> - if (pud_huge(*pud) || !pud_present(*pud)) >>>> - return (pte_t *)pud; >>>> + if (pud_huge(pud) || !pud_present(pud)) >>>> + return (pte_t *)pudp; >>>> =20 >>>> - pmd =3D pmd_offset(pud, addr); >>>> - if (sz !=3D PMD_SIZE && pmd_none(*pmd)) >>>> + pmdp =3D pmd_offset(pudp, addr); >>>> + pmd =3D READ_ONCE(*pmdp); >>>> + if (sz !=3D PMD_SIZE && pmd_none(pmd)) >>>> return NULL; >>>> /* hugepage or swap? */ >>>> - if (pmd_huge(*pmd) || !pmd_present(*pmd)) >>>> - return (pte_t *)pmd; >>>> + if (pmd_huge(pmd) || !pmd_present(pmd)) >>>> + return (pte_t *)pmdp; >>>> =20 >>>> return NULL; >>>> } >>>> --=20 >>>> 1.8.3.1 >>>> >>>> >>> >>> . >>> >> >> >> --=20 >> Regards, >> Longpeng(Mike) >> >=20 --=20 Regards, Longpeng(Mike)