Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755359AbbFLXY4 (ORCPT ); Fri, 12 Jun 2015 19:24:56 -0400 Received: from mail-lb0-f171.google.com ([209.85.217.171]:35161 "EHLO mail-lb0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751590AbbFLXYy (ORCPT ); Fri, 12 Jun 2015 19:24:54 -0400 MIME-Version: 1.0 In-Reply-To: <1434023278-21808-1-git-send-email-dvlasenk@redhat.com> References: <1434023278-21808-1-git-send-email-dvlasenk@redhat.com> From: Andy Lutomirski Date: Fri, 12 Jun 2015 16:24:32 -0700 Message-ID: Subject: Re: [PATCH] x86/asm/entry/32: Slightly better handling of syscall errors in auditing To: Denys Vlasenko Cc: Ingo Molnar , Linus Torvalds , Steven Rostedt , Borislav Petkov , "H. Peter Anvin" , Oleg Nesterov , Frederic Weisbecker , Alexei Starovoitov , Will Drewry , Kees Cook , X86 ML , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3210 Lines: 85 On Thu, Jun 11, 2015 at 4:47 AM, Denys Vlasenko 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 > CC: Linus Torvalds > CC: Steven Rostedt > CC: Ingo Molnar > CC: Borislav Petkov > CC: "H. Peter Anvin" > CC: Andy Lutomirski > CC: Oleg Nesterov > CC: Frederic Weisbecker > CC: Alexei Starovoitov > CC: Will Drewry > CC: Kees Cook > CC: x86@kernel.org > CC: linux-kernel@vger.kernel.org > --- > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/