Re: [PATCH] x86/asm/entry/32: Slightly better handling of syscall errors in auditing

[Andy Lutomirski]

On Thu, Jun 11, 2015 at 4:47 AM, Denys Vlasenko <dvlasenk@xxxxxxxxxx> wrote:
> "setbe %al" insn has a register merge stall: it needs to combine
> previous %eax value with new value for the lowest byte.
> Subsequent "movzbl %al,%edi" in turn depends on its completion.
>
> This patch replaces "setbe %al + movzbl %al,%edi" pair of insns
> with "xor %edi,%edi" before the comparison, and conditional "inc %edi".
>
> This results in the same value of %edi as produced by old code,
> but first insn has no dependencies, and we end up with having
> only one insn with deps which executes only if %eax contains error
> return, and both insns are shorter: 2 bytes each versus 3 bytes each.
>
> (The old code was inherited from 32-bit code, where it allowed to avoid
> a conditional jump. Here we have to use a jump anyway).
>
> Signed-off-by: Denys Vlasenko <dvlasenk@xxxxxxxxxx>
> CC: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> CC: Steven Rostedt <rostedt@xxxxxxxxxxx>
> CC: Ingo Molnar <mingo@xxxxxxxxxx>
> CC: Borislav Petkov <bp@xxxxxxxxx>
> CC: "H. Peter Anvin" <hpa@xxxxxxxxx>
> CC: Andy Lutomirski <luto@xxxxxxxxxxxxxx>
> CC: Oleg Nesterov <oleg@xxxxxxxxxx>
> CC: Frederic Weisbecker <fweisbec@xxxxxxxxx>
> CC: Alexei Starovoitov <ast@xxxxxxxxxxxx>
> CC: Will Drewry <wad@xxxxxxxxxxxx>
> CC: Kees Cook <keescook@xxxxxxxxxxxx>
> CC: x86@xxxxxxxxxx
> CC: linux-kernel@xxxxxxxxxxxxxxx
> ---
>  arch/x86/entry/entry_64_compat.S | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
> index bb187a6..96f33a4 100644
> --- a/arch/x86/entry/entry_64_compat.S
> +++ b/arch/x86/entry/entry_64_compat.S
> @@ -213,12 +213,13 @@ sysexit_from_sys_call:
>         jnz     ia32_ret_from_sys_call
>         TRACE_IRQS_ON
>         ENABLE_INTERRUPTS(CLBR_NONE)
> +       xor     %edi, %edi
>         movl    %eax, %esi              /* second arg, syscall return value */
>         cmpl    $-MAX_ERRNO, %eax       /* is it an error ? */
>         jbe     1f

We go here if !be, which is allegedly the error case, which confuses
me, because setbe will set al (and hence edi) if be, which is also
claimed to be the error case.  Ignoring the comments for now...

>         movslq  %eax, %rsi              /* if error sign extend to 64 bits */
> -1:     setbe   %al                     /* 1 if error, 0 if not */
> -       movzbl  %al, %edi               /* zero-extend that into %edi */

Old code: edi == 1 if be and edi == 0 if !be.

> +       inc     %edi

New code: edi == 1 if !be and edi == 1 if be.

So I think that both the comment and the new code are wrong.

Am I just confused?

--Andy

> +1:     /* edi: 1 if error, 0 if not */
>         call    __audit_syscall_exit
>         movq    RAX(%rsp), %rax         /* reload syscall return value */
>         movl    $(_TIF_ALLWORK_MASK & ~_TIF_SYSCALL_AUDIT), %edi
> --
> 1.8.1.4
>



-- 
Andy Lutomirski
AMA Capital Management, LLC
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/