linux-kernel - Re: [PATCH v1 5/5] powerpc/fault: Perform exception fixup in do_page

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <1599555291.xys33gyt02.astroid@bobo.none>
Date:   Tue, 08 Sep 2020 19:13:37 +1000
From:   Nicholas Piggin <npiggin@...il.com>
To:     Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        Michael Ellerman <mpe@...erman.id.au>,
        Paul Mackerras <paulus@...ba.org>
Cc:     linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v1 5/5] powerpc/fault: Perform exception fixup in
 do_page_fault()

Excerpts from Christophe Leroy's message of August 7, 2020 3:15 am:
> Exception fixup doesn't require the heady full regs saving,
                                      heavy

> do it from do_page_fault() directly.
> 
> For that, split bad_page_fault() in two parts.
> 
> As bad_page_fault() can also be called from other places than
> handle_page_fault(), it will still perform exception fixup and
> fallback on __bad_page_fault().
> 
> handle_page_fault() directly calls __bad_page_fault() as the
> exception fixup will now be done by do_page_fault()

Looks good. We can probably get rid of bad_page_fault completely after 
this too.

Hmm, the alignment exception might(?) hit user copies if the user points 
it to CI memory. Then you could race and the memory gets unmapped. In 
that case the exception table check might be better to be explicit there
with comments.

The first call in do_hash_fault is not required (copy user will never
be in nmi context). The second one and the one in slb_fault could be
made explicit too. Anyway for now this is fine.

Thanks,
Nick

Reviewed-by: Nicholas Piggin <npiggin@...il.com>


> 
> Signed-off-by: Christophe Leroy <christophe.leroy@...roup.eu>
> ---
>  arch/powerpc/kernel/entry_32.S       |  2 +-
>  arch/powerpc/kernel/exceptions-64e.S |  2 +-
>  arch/powerpc/kernel/exceptions-64s.S |  2 +-
>  arch/powerpc/mm/fault.c              | 33 ++++++++++++++++++++--------
>  4 files changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
> index f4d0af8e1136..c198786591f9 100644
> --- a/arch/powerpc/kernel/entry_32.S
> +++ b/arch/powerpc/kernel/entry_32.S
> @@ -678,7 +678,7 @@ handle_page_fault:
>  	mr	r5,r3
>  	addi	r3,r1,STACK_FRAME_OVERHEAD
>  	lwz	r4,_DAR(r1)
> -	bl	bad_page_fault
> +	bl	__bad_page_fault
>  	b	ret_from_except_full
>  
>  #ifdef CONFIG_PPC_BOOK3S_32
> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
> index d9ed79415100..dd9161ea5da8 100644
> --- a/arch/powerpc/kernel/exceptions-64e.S
> +++ b/arch/powerpc/kernel/exceptions-64e.S
> @@ -1024,7 +1024,7 @@ storage_fault_common:
>  	mr	r5,r3
>  	addi	r3,r1,STACK_FRAME_OVERHEAD
>  	ld	r4,_DAR(r1)
> -	bl	bad_page_fault
> +	bl	__bad_page_fault
>  	b	ret_from_except
>  
>  /*
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index f7d748b88705..2cb3bcfb896d 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -3254,7 +3254,7 @@ handle_page_fault:
>  	mr	r5,r3
>  	addi	r3,r1,STACK_FRAME_OVERHEAD
>  	ld	r4,_DAR(r1)
> -	bl	bad_page_fault
> +	bl	__bad_page_fault
>  	b	interrupt_return
>  
>  /* We have a data breakpoint exception - handle it */
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index edde169ba3a6..bd6e397eb84a 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -542,10 +542,20 @@ NOKPROBE_SYMBOL(__do_page_fault);
>  int do_page_fault(struct pt_regs *regs, unsigned long address,
>  		  unsigned long error_code)
>  {
> +	const struct exception_table_entry *entry;
>  	enum ctx_state prev_state = exception_enter();
>  	int rc = __do_page_fault(regs, address, error_code);
>  	exception_exit(prev_state);
> -	return rc;
> +	if (likely(!rc))
> +		return 0;
> +
> +	entry = search_exception_tables(regs->nip);
> +	if (unlikely(!entry))
> +		return rc;
> +
> +	instruction_pointer_set(regs, extable_fixup(entry));
> +
> +	return 0;
>  }
>  NOKPROBE_SYMBOL(do_page_fault);
>  
> @@ -554,17 +564,10 @@ NOKPROBE_SYMBOL(do_page_fault);
>   * It is called from the DSI and ISI handlers in head.S and from some
>   * of the procedures in traps.c.
>   */
> -void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
> +void __bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
>  {
> -	const struct exception_table_entry *entry;
>  	int is_write = page_fault_is_write(regs->dsisr);
>  
> -	/* Are we prepared to handle this fault?  */
> -	if ((entry = search_exception_tables(regs->nip)) != NULL) {
> -		regs->nip = extable_fixup(entry);
> -		return;
> -	}
> -
>  	/* kernel has accessed a bad area */
>  
>  	switch (TRAP(regs)) {
> @@ -598,3 +601,15 @@ void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
>  
>  	die("Kernel access of bad area", regs, sig);
>  }
> +
> +void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
> +{
> +	const struct exception_table_entry *entry;
> +
> +	/* Are we prepared to handle this fault?  */
> +	entry = search_exception_tables(instruction_pointer(regs));
> +	if (entry)
> +		instruction_pointer_set(regs, extable_fixup(entry));
> +	else
> +		__bad_page_fault(regs, address, sig);
> +}
> -- 
> 2.25.0
> 
>