linux-kernel - Re: [PATCH v2] x86/entry_64.S: introduce prepare_error

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20160117094723.GB8549@pd.tnic>
Date:	Sun, 17 Jan 2016 10:47:23 +0100
From:	Borislav Petkov <bp@...e.de>
To:	Alexander Kuleshov <kuleshovmail@...il.com>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H . Peter Anvin" <hpa@...or.com>, x86@...nel.org,
	Andy Lutomirski <luto@...nel.org>,
	Denys Vlasenko <dvlasenk@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] x86/entry_64.S: introduce prepare_error_code macro

On Sun, Jan 17, 2016 at 01:41:18PM +0600, Alexander Kuleshov wrote:
> We need to put an error code to the %rsi if an exception provides
> it, before the call of an exception handler. We do it in the idtentry
> macro in two places.
> 
> This patch introduces prepare_error_code macro which will check existence
> of an error code and put it to %rsi from ORIG_RAX if it exists, or just
> clears %esi if an error code does not exist to prevent code duplication
> in the idtentry macro.
> 
> Signed-off-by: Alexander Kuleshov <kuleshovmail@...il.com>
> ---
> 
> v2: indentation fixed
> 
>  arch/x86/entry/entry_64.S | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index 9d34d3c..e2b1e79 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -722,6 +722,15 @@ apicinterrupt IRQ_WORK_VECTOR			irq_work_interrupt		smp_irq_work_interrupt
>   */
>  #define CPU_TSS_IST(x) PER_CPU_VAR(cpu_tss) + (TSS_ist + ((x) - 1) * 8)
>  
> +	.macro prepare_error_code has_error_code:req
> +	.if \has_error_code
> +	movq	ORIG_RAX(%rsp), %rsi		/* get error code */
> +	movq	$-1, ORIG_RAX(%rsp)		/* no syscall to restart */
> +	.else
> +	xorl	%esi, %esi			/* no error code */
> +	.endif
> +	.endm
> +
>  .macro idtentry sym do_sym has_error_code:req paranoid=0 shift_ist=-1
>  ENTRY(\sym)
>  	/* Sanity check */
> @@ -759,12 +768,7 @@ ENTRY(\sym)
>  
>  	movq	%rsp, %rdi			/* pt_regs pointer */
>  
> -	.if \has_error_code
> -	movq	ORIG_RAX(%rsp), %rsi		/* get error code */
> -	movq	$-1, ORIG_RAX(%rsp)		/* no syscall to restart */
> -	.else
> -	xorl	%esi, %esi			/* no error code */
> -	.endif
> +	prepare_error_code \has_error_code      /* %rsi -> error code */

So this doesn't make it more readable to me, but less. And if it weren't
for the comment to the right which says that error code goes into %rsi
(yes, and not %rsi -> error code) I would have no idea what the code
fragment

	...
	movq %rsp, %rdi
	prepare_error_code \has_error_code
	...

does and I'd have to go look up the macro.

Basically, this patch does the opposite of what the suggested traps.c
change does. See what I mean?

IOW, what the rule should be - and mind you, this is only my opinion -
the code should be as readable as possible. One should be able to look
at any function and say, aha, this is what it does there.

Why?

Well, the most frequent reason why anyone would need to look at code is
when one is trying to figure out a bug and is following the code flow to
refresh in one's head what's going on.

Now, if the code is full if funky macros which one has to look up first
to understand what they're doing and then continue on, then we clearly
haven't done our job right. If there are no comments at all and the flow
is not trivial, we too, haven't done everything right.

So IMO, the most important strategy we should be pursuing is being able
to read and understand the code as quickly as possible. And not save
some lines with a funny macro name or silly traps.c helpers, or,... you
name it. Ok?

I hope it gets across exactly as I mean it.

Thanks.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--