From nobody Tue Sep 06 15:30:01 2022 X-Original-To: dev-commits-src-all@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 4MMTpq1jsjz4c3Bp; Tue, 6 Sep 2022 15:30:11 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (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 did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4MMTpp05F7z49V7; Tue, 6 Sep 2022 15:30:09 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.16.1/8.16.1) with ESMTPS id 286FU1MO088342 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NO); Tue, 6 Sep 2022 18:30:05 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by tom.home (8.16.1/8.16.1/Submit) id 286FU1oM088338; Tue, 6 Sep 2022 18:30:01 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Tue, 6 Sep 2022 18:30:01 +0300 From: Konstantin Belousov To: =?utf-8?Q?T=C4=B3l?= Coosemans Cc: pho@freebsd.org, 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: References: <202208241925.27OJP9Fh069091@gitrepo.freebsd.org> <20220906171826.1629cfcf@FreeBSD.org> List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20220906171826.1629cfcf@FreeBSD.org> X-Spam-Status: No, score=-1.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FORGED_GMAIL_RCVD,FREEMAIL_FROM, NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on tom.home X-Rspamd-Queue-Id: 4MMTpp05F7z49V7 X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org; dkim=none; dmarc=fail reason="No valid SPF, No valid DKIM" header.from=gmail.com (policy=none); spf=softfail (mx1.freebsd.org: 2001:470:d5e7:1::1 is neither permitted nor denied by domain of kostikbel@gmail.com) smtp.mailfrom=kostikbel@gmail.com X-Spamd-Result: default: False [-2.97 / 15.00]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_MEDIUM(-1.00)[-0.998]; NEURAL_HAM_SHORT(-0.98)[-0.975]; DMARC_POLICY_SOFTFAIL(0.10)[gmail.com : No valid SPF, No valid DKIM,none]; MIME_GOOD(-0.10)[text/plain]; MLMMJ_DEST(0.00)[dev-commits-src-all@freebsd.org,dev-commits-src-main@freebsd.org]; FROM_EQ_ENVFROM(0.00)[]; R_DKIM_NA(0.00)[]; ASN(0.00)[asn:6939, ipnet:2001:470::/32, country:US]; MIME_TRACE(0.00)[0:+]; RCVD_TLS_LAST(0.00)[]; RCVD_COUNT_THREE(0.00)[3]; RCPT_COUNT_FIVE(0.00)[5]; R_SPF_SOFTFAIL(0.00)[~all:c]; TO_DN_SOME(0.00)[]; FROM_HAS_DN(0.00)[]; FREEMAIL_ENVFROM(0.00)[gmail.com]; FREEMAIL_FROM(0.00)[gmail.com]; HAS_XAW(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; ARC_NA(0.00)[] X-ThisMailContainsUnwantedMimeParts: N On Tue, Sep 06, 2022 at 05:18:26PM +0200, Tijl Coosemans wrote: > 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 purpose of the patch was to change some tripple faults into normal panics. The check for %eip belonging to the trampoline area is too broad to be correct. I suspect you see that leftover panics, which I am working on right now. > > 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. I do not think that the patch is correct, and I even surprised that you do not see a sporadic reboots with it applied (do you?). When you change address space to do the fast bcopy, kernel stack gets unmapped. So if the page fault occurs because user page is not resident, fault must cause tripple fault and reboot the system. The idea about too small trampoline stack might have some merits, did you tried to increase the trampoline stack size? I am currently reworking the copyin/copyout_fast to avoid pushing the bcopy args into trampoline stack, but there is one bug not yet fixed in the change. See https://kib.kiev.ua/git/gitweb.cgi?p=deviant3.git;a=shortlog;h=refs/heads/ast > 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