linux-kernel - Re: [PATCH 2/2] ARC: don't align ret_from

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: <448ba208-56a5-d8b2-9675-7be03b872c23@synopsys.com>
Date:   Wed, 11 Mar 2020 17:38:47 +0000
From:   Vineet Gupta <Vineet.Gupta1@...opsys.com>
To:     Eugeniy Paltsev <Eugeniy.Paltsev@...opsys.com>,
        "linux-snps-arc@...ts.infradead.org" 
        <linux-snps-arc@...ts.infradead.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Alexey Brodkin" <Alexey.Brodkin@...opsys.com>
Subject: Re: [PATCH 2/2] ARC: don't align ret_from_exception function

On 3/11/20 9:26 AM, Eugeniy Paltsev wrote:
> ARC have a tricky implemented ret_from_exception function.
> It is written on ASM and can be called like regular function.
> However it has another 'entry point' as it can be called as a
> continuation of EV_Trap function.

It is not really intended / needed to be called form outside ASM - but has to be
spread across 2 asm files as it is mostly target isa independent, while the
callers are in separate target isa specific files.
The ENTRY/END annotations are simply for the dwarf unwinder to stop gracefully.

> As we declare "ret_from_exception" using ENTRY macro it may align
> "ret_from_exception" by 4 bytes by adding padding before it.
> "ret_from_exception" doesn't require alignment by 4 bytes
> (as it doesn't go to vector table, etc...) so let's avoid aligning
> it by switch from ENTRY (which is alias for SYM_FUNC_START) to
> SYM_FUNC_START_NOALIGN macro.

I would like to keep it aligned.

ARC700 definitely has penalty for unaligned branch targets, so it will definitely
suffer there.

For HS, unaligned branch targets have no penalty (for the general case atleast),
but if it does get unaligned, the entire entry prologue code will be - i.e. each
one of the subsequent 130 or so instructions. That doesn't seem like a good idea
in general to me.

What's weird is I never hit the original problem you ran into - are you using a
toolchain with the branch relaxation stuff  ?
I faked it using a nop_s and the SYM_FUNC_START_NOALIGN( ) annotation and can see
all instructions getting unaligned.

Before;

921238e4 <ret_from_exception>:
921238e4:    ld    r8,[sp,124]
921238e8:    bbit0.nt    r8,0x7,212   
...
92123ac8:    rtie
92123acc <debug_marker_ds>:
92123acc:    ld    r2,[0x927d81d8]
92123ad4:    add    r2,r2,0x1
92123ad8:    st    r2,[0x927d81d8]
92123ae0:    bmskn    r11,r10,0xf
92123ae4:    sr    r11,[aux_irq_act]
92123ae8:    b    -140    ;92123a5c

After:

9212393c:    nop_s
9212393e <ret_from_exception>:
9212393e:    ld    r8,[sp,124]
92123942:    bbit0.nt    r8,0x7,214
...
92123b22:    rtie
92123b26 <debug_marker_ds>:
92123b26:    ld    r2,[0x927d81d8]
92123b2e:    add    r2,r2,0x1
92123b32:    st    r2,[0x927d81d8]
92123b3a:    bmskn    r11,r10,0xf
92123b3e:    sr    r11,[aux_irq_act]
92123b42:    b    -138    ;92123ab6 <debug_marker_syscall>
92123b46:    nop_s

> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@...opsys.com>
> ---
>  arch/arc/kernel/entry-arcv2.S | 2 +-
>  arch/arc/kernel/entry.S       | 3 +--
>  2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arc/kernel/entry-arcv2.S b/arch/arc/kernel/entry-arcv2.S
> index 12d5f12d10d2..d482e1507f56 100644
> --- a/arch/arc/kernel/entry-arcv2.S
> +++ b/arch/arc/kernel/entry-arcv2.S
> @@ -260,4 +260,4 @@ debug_marker_ds:
>  	sr	r11, [AUX_IRQ_ACT]
>  	b	.Lexcept_ret
>  
> -END(ret_from_exception)
> +SYM_FUNC_END(ret_from_exception)
> diff --git a/arch/arc/kernel/entry.S b/arch/arc/kernel/entry.S
> index 60406ec62eb8..79409b518a09 100644
> --- a/arch/arc/kernel/entry.S
> +++ b/arch/arc/kernel/entry.S
> @@ -280,7 +280,7 @@ END(EV_Trap)
>  ;
>  ; If ret to user mode do we need to handle signals, schedule() et al.
>  
> -ENTRY(ret_from_exception)
> +SYM_FUNC_START_NOALIGN(ret_from_exception)
>  
>  	; Pre-{IRQ,Trap,Exception} K/U mode from pt_regs->status32
>  	ld  r8, [sp, PT_status32]   ; returning to User/Kernel Mode
> @@ -373,4 +373,3 @@ resume_kernel_mode:
>  	b	.Lrestore_regs
>  
>  ##### DONT ADD CODE HERE - .Lrestore_regs actually follows in entry-<isa>.S
> -