Re: git: 95f773e59482 - main - i386 copyout_fast: improve detection of a fault on accessing userspace

On Wed, 24 Aug 2022 19:25:09 GMT Konstantin Belousov <kib@FreeBSD.org>
wrote:
> The branch main has been updated by kib:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=95f773e59482b1a3462d2fe3901532d51fb053b3
> 
> commit 95f773e59482b1a3462d2fe3901532d51fb053b3
> Author:     Konstantin Belousov <kib@FreeBSD.org>
> AuthorDate: 2022-08-09 00:56:54 +0000
> Commit:     Konstantin Belousov <kib@FreeBSD.org>
> 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.