From nobody Tue Sep 06 15:18:26 2022 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4MMTYM4dGrz4c1vR; Tue, 6 Sep 2022 15:18:31 +0000 (UTC) (envelope-from tijl@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4MMTYM43vHz47Vt; Tue, 6 Sep 2022 15:18:31 +0000 (UTC) (envelope-from tijl@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1662477511; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=JU3EPElC2poZK1vRihMKvUMW90nmo6USTVvySsHQ4xE=; b=TjZwUr5t1+z98GB5HVWxLqKEmh1tyCI4eVRnl3J7SFaloseEjfjYO37lyrtko4HE3oEfaO Fh5gLKP73Mz5ZNTQZp2VbwlGEOrzG4hveFusKRkV1mpzkvmngSq3kdExMMnX66gjUbg8/Z O95cSGnljCLVzVBRnOB31DbxigW/i0h1Hrpo1X20IxuUnH5by5/7Nm4m+m+J/mx+xItq0D PWgCWx9xINa5I81JzczZ7JXEa7n9v2NDYY2RH9+ocFG27QaqPks6OtGn1ddKUTR6T4uYwq 2RY4zmkP7hoIEsPmuNERA2K08s1tU0ZVF7Yo1fdpI+QxsDgW07i5gbc1fVWUVg== Received: from localhost (unknown [IPv6:2a02:a03f:894b:4700:20a7:3046:629d:6c01]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) (Authenticated sender: tijl) by smtp.freebsd.org (Postfix) with ESMTPSA id 4MMTYL5vjFz1089; Tue, 6 Sep 2022 15:18:30 +0000 (UTC) (envelope-from tijl@FreeBSD.org) Date: Tue, 6 Sep 2022 17:18:26 +0200 From: =?UTF-8?B?VMSzbA==?= Coosemans To: Konstantin Belousov Cc: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: Re: git: 95f773e59482 - main - i386 copyout_fast: improve detection of a fault on accessing userspace Message-ID: <20220906171826.1629cfcf@FreeBSD.org> In-Reply-To: <202208241925.27OJP9Fh069091@gitrepo.freebsd.org> References: <202208241925.27OJP9Fh069091@gitrepo.freebsd.org> List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="MP_/U.cPUdlq5TwQxEIRzNNcJ.p" ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1662477511; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=JU3EPElC2poZK1vRihMKvUMW90nmo6USTVvySsHQ4xE=; b=ArYlNsEBDS23q048VkE4qn8pZKs6a3KoxWIz6On5ZKu8mMtnQH4tUv2BREOGgSndiJe45G QDMmVxoWeDPgSDfbVQpc8uQhSwyHeu0G7mJmJtVfSPvl61G4qPA7nsHkNDq71C/diY9ltE 7eCy2qsFGPV+IlfelcEVTEwz00E6RwH3Nknum/ehk0cD9a5+4pxxcTjT95U+QQXGTD5oyb vxgpL0J6rd4RVl6yT/QFrewx540noKJugb54tSumxvjHx+BPvox3o6P5EDUQXaIOxTDI3l +w49EZoGbv5PRlf0xnP/jMGsTCMafWE64GJ73kGsIKqA56LQBZX00PPZgr8Kzw== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1662477511; a=rsa-sha256; cv=none; b=oYDo0+j5HbE4SRB1zvQfJAhhXZL2j/LdGzs/tdOkCwKsXbsUS1/mg0m6KB7GGTfRVpi+mg TfmI4IxSpB5V97sP7V2VVgtWXSkh8bgPJ/3nUI3mnXaI3oQ9MAD0allLkRoK+9UgdPDfPo I/jUoN2o1tvXToo5ovV+oEoA4kShc6Hmng1fh9V/5bSO5XAJJ3VJjJH4PYQ76ufAYimKJO 5WpJiWEsFHUMGpD1VK26NuAwsnaCjMxPWTbuTMxOmIaM7vIP8W2YqeDzgag9PbgNDsNC6M 66pugRGEQypQa3/A5cp8Bvjz6yGvFxTdIzF2BlBJR7ixe7REFayMKCOH/dDpaA== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N --MP_/U.cPUdlq5TwQxEIRzNNcJ.p Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Content-Disposition: inline On Wed, 24 Aug 2022 19:25:09 GMT Konstantin Belousov wrote: > The branch main has been updated by kib: > > URL: https://cgit.FreeBSD.org/src/commit/?id=95f773e59482b1a3462d2fe3901532d51fb053b3 > > commit 95f773e59482b1a3462d2fe3901532d51fb053b3 > Author: Konstantin Belousov > AuthorDate: 2022-08-09 00:56:54 +0000 > Commit: Konstantin Belousov > CommitDate: 2022-08-24 19:11:40 +0000 > > i386 copyout_fast: improve detection of a fault on accessing userspace > > Do not blindly account a page fault occuring on the trampoline area, > as the userspace access fault. Check that it occured exactly in the > instruction that does that. > > This avoids unneeded switches of address space on faults not needing the > switch, effectively converting machine resets due to tripple faults, > into regular panics. > > Reviewed by: jhb > Tested by: pho > Sponsored by: The FreeBSD Foundation > MFC after: 1 week > Differential revision: https://reviews.freebsd.org/D36302 > --- > sys/i386/i386/copyout_fast.s | 16 ++++++++-------- > sys/i386/i386/exception.s | 32 ++++++++++++++++++++++++++++---- > 2 files changed, 36 insertions(+), 12 deletions(-) > > diff --git a/sys/i386/i386/copyout_fast.s b/sys/i386/i386/copyout_fast.s > index 715952f5fe20..d1d17f775872 100644 > --- a/sys/i386/i386/copyout_fast.s > +++ b/sys/i386/i386/copyout_fast.s > @@ -93,7 +93,7 @@ ENTRY(copyout_fast) > popl %ecx > popl %edi > popl %esi > - rep; movsb > +pf_x1: rep; movsb > > movl %ebx,%cr3 > movl %eax,%esp > @@ -150,7 +150,7 @@ ENTRY(copyin_fast) > popl %ecx > popl %edi > popl %esi > - rep; movsb > +pf_x2: rep; movsb > > movl %ebx,%cr3 > > @@ -197,7 +197,7 @@ ENTRY(fueword_fast) > cli > movl PCPU(TRAMPSTK),%esp > movl %eax,%cr3 > - movl (%ecx),%eax > +pf_x3: movl (%ecx),%eax > movl %ebx,%cr3 > movl %esi,%esp > sti > @@ -226,7 +226,7 @@ ENTRY(fuword16_fast) > cli > movl PCPU(TRAMPSTK),%esp > movl %eax,%cr3 > - movzwl (%ecx),%eax > +pf_x4: movzwl (%ecx),%eax > movl %ebx,%cr3 > movl %esi,%esp > sti > @@ -252,7 +252,7 @@ ENTRY(fubyte_fast) > cli > movl PCPU(TRAMPSTK),%esp > movl %eax,%cr3 > - movzbl (%ecx),%eax > +pf_x5: movzbl (%ecx),%eax > movl %ebx,%cr3 > movl %esi,%esp > sti > @@ -291,7 +291,7 @@ ENTRY(suword_fast) > cli > movl PCPU(TRAMPSTK),%esp > movl %eax,%cr3 > - movl %edi,(%ecx) > +pf_x6: movl %edi,(%ecx) > movl %ebx,%cr3 > movl %esi,%esp > sti > @@ -319,7 +319,7 @@ ENTRY(suword16_fast) > cli > movl PCPU(TRAMPSTK),%esp > movl %eax,%cr3 > - movw %di,(%ecx) > +pf_x7: movw %di,(%ecx) > movl %ebx,%cr3 > movl %esi,%esp > sti > @@ -348,7 +348,7 @@ ENTRY(subyte_fast) > movl PCPU(TRAMPSTK),%esp > movl %eax,%cr3 > movl %edi,%eax > - movb %al,(%ecx) > +pf_x8: movb %al,(%ecx) > movl %ebx,%cr3 > movl %esi,%esp > sti > diff --git a/sys/i386/i386/exception.s b/sys/i386/i386/exception.s > index f4135548fd81..42e9c474c3cd 100644 > --- a/sys/i386/i386/exception.s > +++ b/sys/i386/i386/exception.s > @@ -130,17 +130,41 @@ IDTVEC(prot) > jmp irettraps > IDTVEC(page) > testl $PSL_VM, TF_EFLAGS-TF_ERR(%esp) > - jnz 1f > + jnz 4f > testb $SEL_RPL_MASK, TF_CS-TF_ERR(%esp) > - jnz 1f > + jnz 4f > cmpl $PMAP_TRM_MIN_ADDRESS, TF_EIP-TF_ERR(%esp) > - jb 1f > + jb 4f > + pushl %eax > + movl TF_EIP-TF_ERR+4(%esp), %eax > + addl $1f, %eax > + call 5f > +1: cmpl $pf_x1, %eax > + je 2f > + cmpl $pf_x2, %eax > + je 2f > + cmpl $pf_x3, %eax > + je 2f > + cmpl $pf_x4, %eax > + je 2f > + cmpl $pf_x5, %eax > + je 2f > + cmpl $pf_x6, %eax > + je 2f > + cmpl $pf_x7, %eax > + je 2f > + cmpl $pf_x8, %eax > + jne 3f > +2: popl %eax > movl %ebx, %cr3 > movl %edx, TF_EIP-TF_ERR(%esp) > addl $4, %esp > iret > -1: pushl $T_PAGEFLT > +3: popl %eax > +4: pushl $T_PAGEFLT > jmp alltraps > +5: subl (%esp), %eax > + retl > IDTVEC(rsvd_pti) > IDTVEC(rsvd) > pushl $0; TRAP(T_RESERVED) I'm sporadically seeing a panic after this commit. It's caused by a page fault during the second rep; movsb in copyin_fast (copying between copyout_buf and the kernel). When I add the same special treatment as above for this instruction the panic goes away. The panics happened while in X and the crash dump doesn't seem to contain the trampoline stack so I ended up rewriting copyin_fast and copyout_fast so the copying between the kernel and copyout_buf ran on the kernel stack instead of the trampoline stack (see attached patch). Now with this patch the panics are also gone so I suspect the problem is simply that the trampoline stack is too small to handle some page faults. So, is this patch correct then? I'm not sure it's actually safe to handle page faults in this context with interrupts disabled. --MP_/U.cPUdlq5TwQxEIRzNNcJ.p Content-Type: text/x-patch Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=i386-copyout_fast.patch diff --git a/sys/i386/i386/copyout_fast.s b/sys/i386/i386/copyout_fast.s index d1d17f775872..4b7d4c293466 100644 --- a/sys/i386/i386/copyout_fast.s +++ b/sys/i386/i386/copyout_fast.s @@ -46,53 +46,28 @@ ENTRY(copyout_fast) pushl %edi pushl %ebx - movl $copyout_fault,%edx + movl PCPU(CURPCB),%edx + movl 16(%ebp),%ecx /* len */ + movl 8(%ebp),%esi /* kaddr */ + movl PCPU(COPYOUT_BUF),%edi movl 20(%ebp),%ebx /* KCR3 */ - - movl PCPU(CURPCB),%eax - movl PCB_CR3(%eax),%edi + movl PCB_CR3(%edx),%edx cli - movl PCPU(TRAMPSTK),%esi - movl PCPU(COPYOUT_BUF),%eax - subl $4,%esi - movl %eax,(%esi) - movl 12(%ebp),%eax /* udaddr */ - subl $4,%esi - movl %eax,(%esi) - movl 16(%ebp),%eax /* len */ - subl $4,%esi - movl %eax,(%esi) - - subl $4, %esi - movl %edi, (%esi) + /* bcopy(%esi = kaddr, %edi = PCPU(copyout_buf), %ecx = len) */ + rep; movsb - movl 8(%ebp),%eax /* kaddr */ - subl $4,%esi - movl %eax,(%esi) - movl PCPU(COPYOUT_BUF),%eax - subl $4,%esi - movl %eax,(%esi) - movl 16(%ebp),%eax /* len */ - subl $4,%esi - movl %eax,(%esi) + movl 16(%ebp),%ecx /* len */ + movl PCPU(COPYOUT_BUF),%esi + movl 12(%ebp),%edi /* udaddr */ movl %esp,%eax - movl %esi,%esp - - /* bcopy(%esi = kaddr, %edi = PCPU(copyout_buf), %ecx = len) */ - popl %ecx - popl %edi - popl %esi - rep; movsb + movl PCPU(TRAMPSTK),%esp + movl %edx,%cr3 - popl %edi - movl %edi,%cr3 + movl $copyout_fault,%edx /* bcopy(%esi = PCPU(copyout_buf), %edi = udaddr, %ecx = len) */ - popl %ecx - popl %edi - popl %esi pf_x1: rep; movsb movl %ebx,%cr3 @@ -114,53 +89,33 @@ ENTRY(copyin_fast) pushl %edi pushl %ebx - movl $copyout_fault,%edx + movl PCPU(CURPCB),%edx + movl 16(%ebp),%ecx /* len */ + movl 8(%ebp),%esi /* udaddr */ + movl PCPU(COPYOUT_BUF),%edi movl 20(%ebp),%ebx /* KCR3 */ + movl PCB_CR3(%edx),%edx - movl PCPU(CURPCB),%eax - movl PCB_CR3(%eax),%edi - + movl %esp,%eax cli - movl PCPU(TRAMPSTK),%esi - movl PCPU(COPYOUT_BUF),%eax - subl $4,%esi - movl %eax,(%esi) - movl 12(%ebp),%eax /* kaddr */ - subl $4,%esi - movl %eax,(%esi) - movl 16(%ebp),%eax /* len */ - subl $4,%esi - movl %eax,(%esi) - - movl 8(%ebp),%eax /* udaddr */ - subl $4,%esi - movl %eax,(%esi) - movl PCPU(COPYOUT_BUF),%eax - subl $4,%esi - movl %eax,(%esi) - movl 16(%ebp),%eax /* len */ - subl $4,%esi - movl %eax,(%esi) + movl PCPU(TRAMPSTK),%esp + movl %edx,%cr3 - movl %esp,%eax - movl %esi,%esp - movl %edi,%cr3 + movl $copyout_fault,%edx /* bcopy(%esi = udaddr, %edi = PCPU(copyout_buf), %ecx = len) */ - popl %ecx - popl %edi - popl %esi pf_x2: rep; movsb movl %ebx,%cr3 + movl %eax,%esp + + movl 16(%ebp),%ecx /* len */ + movl PCPU(COPYOUT_BUF),%esi + movl 12(%ebp),%edi /* kaddr */ /* bcopy(%esi = PCPU(copyout_buf), %edi = kaddr, %ecx = len) */ - popl %ecx - popl %edi - popl %esi rep; movsb - movl %eax,%esp sti xorl %eax,%eax --MP_/U.cPUdlq5TwQxEIRzNNcJ.p--