Re: [PATCH -v2] Audit: push audit success and retcode into arch ptrace.h
On Fri, Jun 03, 2011 at 06:04:51PM -0400, Eric Paris wrote: Thanks, this looks good compiles, so: Acked-by: Ralf Baechle r...@linux-mips.org I will rebase my pending MIPS audit patches on top of this patch and resend. Thanks, Ralf ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH -v2] Audit: push audit success and retcode into arch ptrace.h
On 06/07, Eric Paris wrote: On Tue, 2011-06-07 at 19:19 +0200, Oleg Nesterov wrote: With or without this patch, can't we call audit_syscall_exit() twice if there is something else in _TIF_WORK_SYSCALL_EXIT mask apart from SYSCALL_AUDIT ? First time it is called from asm, then from syscall_trace_leave(), no? For example. The task has TIF_SYSCALL_AUDIT and nothing else, it does system_call-auditsys-system_call_fastpath. What if it gets, say, TIF_SYSCALL_TRACE before ret_from_sys_call? No harm is done calling twice. The first call will do the real work and cleanup. It will set a flag in the audit data that the work has been done (in_syscall == 0) thus the second call will then not do any real work and won't have anything to clean up. Hmm... and I assume context-previous != NULL is not possible on x86_64. OK, thanks. And I guess, all CONFIG_AUDITSYSCALL code in entry.S is only needed to microoptimize the case when TIF_SYSCALL_AUDIT is the only reason for the slow path. I wonder if it really makes the measureble difference... Oleg. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH -v2] Audit: push audit success and retcode into arch ptrace.h
On Wed, 2011-06-08 at 18:36 +0200, Oleg Nesterov wrote: On 06/07, Eric Paris wrote: On Tue, 2011-06-07 at 19:19 +0200, Oleg Nesterov wrote: With or without this patch, can't we call audit_syscall_exit() twice if there is something else in _TIF_WORK_SYSCALL_EXIT mask apart from SYSCALL_AUDIT ? First time it is called from asm, then from syscall_trace_leave(), no? For example. The task has TIF_SYSCALL_AUDIT and nothing else, it does system_call-auditsys-system_call_fastpath. What if it gets, say, TIF_SYSCALL_TRACE before ret_from_sys_call? No harm is done calling twice. The first call will do the real work and cleanup. It will set a flag in the audit data that the work has been done (in_syscall == 0) thus the second call will then not do any real work and won't have anything to clean up. Hmm... and I assume context-previous != NULL is not possible on x86_64. OK, thanks. And I guess, all CONFIG_AUDITSYSCALL code in entry.S is only needed to microoptimize the case when TIF_SYSCALL_AUDIT is the only reason for the slow path. I wonder if it really makes the measureble difference... All I know is what Roland put in the changelog: Avoiding the iret return path when syscall audit is enabled helps performance a lot. I believe this was a result of Fedora starting auditd by default and then Linus bitching about how slow a null syscall in a tight loop was. It was an optimization for a microbenchmark. How much it affects things on a real syscall that does real work is probably going to be determined by how much work is done in the syscall. (or just disable auditd in userspace) -Eric ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH -v2] Audit: push audit success and retcode into arch ptrace.h
On 06/08, Eric Paris wrote: On Wed, 2011-06-08 at 18:36 +0200, Oleg Nesterov wrote: And I guess, all CONFIG_AUDITSYSCALL code in entry.S is only needed to microoptimize the case when TIF_SYSCALL_AUDIT is the only reason for the slow path. I wonder if it really makes the measureble difference... All I know is what Roland put in the changelog: Avoiding the iret return path when syscall audit is enabled helps performance a lot. I believe this was a result of Fedora starting auditd by default and then Linus bitching about how slow a null syscall in a tight loop was. It was an optimization for a microbenchmark. How much it affects things on a real syscall that does real work is probably going to be determined by how much work is done in the syscall. and probably by how much work is done in audit_syscall_entry/exit. OK. Thanks a lot Eric for your explanations. Oleg. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH -v2] Audit: push audit success and retcode into arch ptrace.h
On 06/08, Oleg Nesterov wrote: OK. Thanks a lot Eric for your explanations. Yes. but may I ask another one? Shouldn't copy_process()-audit_alloc(tsk) path do clear_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT) if it doesn't set tsk-audit_context? I can be easily wrong, but afaics otherwise the child can run with TIF_SYSCALL_AUDIT bit copied from parent's thread_info by dup_task_struct()-setup_thread_stack() and without -audit_context, right? For what? Any other reason why audit_syscall_entry() checks context != NULL? IOW. Any reason the patch below is wrong? I am just curious, thanks. Oleg. --- x/kernel/auditsc.c +++ x/kernel/auditsc.c @@ -885,6 +885,8 @@ int audit_alloc(struct task_struct *tsk) if (likely(!audit_ever_enabled)) return 0; /* Return if not auditing. */ + clear_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT); + state = audit_filter_task(tsk, key); if (likely(state == AUDIT_DISABLED)) return 0; @@ -1591,9 +1593,7 @@ void audit_syscall_entry(int arch, int m struct audit_context *context = tsk-audit_context; enum audit_state state; - if (unlikely(!context)) - return; - + BUG_ON(!context); /* * This happens only on certain architectures that make system * calls in kernel_thread via the entry.S interface, instead of ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH -v2] Audit: push audit success and retcode into arch ptrace.h
On 06/03, Eric Paris wrote: The audit system previously expected arches calling to audit_syscall_exit to supply as arguments if the syscall was a success and what the return code was. Audit also provides a helper AUDITSC_RESULT which was supposed to simplify things by converting from negative retcodes to an audit internal magic value stating success or failure. This helper was wrong and could indicate that a valid pointer returned to userspace was a failed syscall. The fix is to fix the layering foolishness. We now pass audit_syscall_exit a struct pt_reg and it in turns calls back into arch code to collect the return value and to determine if the syscall was a success or failure. We also define a generic is_syscall_success() macro which determines success/failure based on if the value is -MAX_ERRNO. This works for arches like x86 which do not use a separate mechanism to indicate syscall failure. I know nothing about audit, but the patch looks fine to me. But I have a bit off-topic question, diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S index 8a445a0..b7b1f88 100644 --- a/arch/x86/kernel/entry_64.S +++ b/arch/x86/kernel/entry_64.S @@ -53,6 +53,7 @@ #include asm/paravirt.h #include asm/ftrace.h #include asm/percpu.h +#include linux/err.h /* Avoid __ASSEMBLER__'ifying linux/audit.h just for this. */ #include linux/elf-em.h @@ -564,17 +565,16 @@ auditsys: jmp system_call_fastpath /* - * Return fast path for syscall audit. Call audit_syscall_exit() + * Return fast path for syscall audit. Call __audit_syscall_exit() * directly and then jump back to the fast path with TIF_SYSCALL_AUDIT * masked off. */ sysret_audit: movq RAX-ARGOFFSET(%rsp),%rsi /* second arg, syscall return value */ - cmpq $0,%rsi/* is it 0? */ - setl %al/* 1 if so, 0 if not */ + cmpq $-MAX_ERRNO,%rsi /* is it -MAX_ERRNO? */ + setbe %al /* 1 if so, 0 if not */ movzbl %al,%edi /* zero-extend that into %edi */ - inc %edi /* first arg, 0-1(AUDITSC_SUCCESS), 1-2(AUDITSC_FAILURE) */ - call audit_syscall_exit + call __audit_syscall_exit With or without this patch, can't we call audit_syscall_exit() twice if there is something else in _TIF_WORK_SYSCALL_EXIT mask apart from SYSCALL_AUDIT ? First time it is called from asm, then from syscall_trace_leave(), no? For example. The task has TIF_SYSCALL_AUDIT and nothing else, it does system_call-auditsys-system_call_fastpath. What if it gets, say, TIF_SYSCALL_TRACE before ret_from_sys_call? Oleg. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH -v2] Audit: push audit success and retcode into arch ptrace.h
On Tue, 2011-06-07 at 19:19 +0200, Oleg Nesterov wrote: On 06/03, Eric Paris wrote: The audit system previously expected arches calling to audit_syscall_exit to supply as arguments if the syscall was a success and what the return code was. Audit also provides a helper AUDITSC_RESULT which was supposed to simplify things by converting from negative retcodes to an audit internal magic value stating success or failure. This helper was wrong and could indicate that a valid pointer returned to userspace was a failed syscall. The fix is to fix the layering foolishness. We now pass audit_syscall_exit a struct pt_reg and it in turns calls back into arch code to collect the return value and to determine if the syscall was a success or failure. We also define a generic is_syscall_success() macro which determines success/failure based on if the value is -MAX_ERRNO. This works for arches like x86 which do not use a separate mechanism to indicate syscall failure. I know nothing about audit, but the patch looks fine to me. But I have a bit off-topic question, diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S index 8a445a0..b7b1f88 100644 --- a/arch/x86/kernel/entry_64.S +++ b/arch/x86/kernel/entry_64.S @@ -53,6 +53,7 @@ #include asm/paravirt.h #include asm/ftrace.h #include asm/percpu.h +#include linux/err.h /* Avoid __ASSEMBLER__'ifying linux/audit.h just for this. */ #include linux/elf-em.h @@ -564,17 +565,16 @@ auditsys: jmp system_call_fastpath /* -* Return fast path for syscall audit. Call audit_syscall_exit() +* Return fast path for syscall audit. Call __audit_syscall_exit() * directly and then jump back to the fast path with TIF_SYSCALL_AUDIT * masked off. */ sysret_audit: movq RAX-ARGOFFSET(%rsp),%rsi /* second arg, syscall return value */ - cmpq $0,%rsi/* is it 0? */ - setl %al/* 1 if so, 0 if not */ + cmpq $-MAX_ERRNO,%rsi /* is it -MAX_ERRNO? */ + setbe %al /* 1 if so, 0 if not */ movzbl %al,%edi /* zero-extend that into %edi */ - inc %edi /* first arg, 0-1(AUDITSC_SUCCESS), 1-2(AUDITSC_FAILURE) */ - call audit_syscall_exit + call __audit_syscall_exit With or without this patch, can't we call audit_syscall_exit() twice if there is something else in _TIF_WORK_SYSCALL_EXIT mask apart from SYSCALL_AUDIT ? First time it is called from asm, then from syscall_trace_leave(), no? For example. The task has TIF_SYSCALL_AUDIT and nothing else, it does system_call-auditsys-system_call_fastpath. What if it gets, say, TIF_SYSCALL_TRACE before ret_from_sys_call? No harm is done calling twice. The first call will do the real work and cleanup. It will set a flag in the audit data that the work has been done (in_syscall == 0) thus the second call will then not do any real work and won't have anything to clean up. -Eric ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH -v2] Audit: push audit success and retcode into arch ptrace.h
From: Eric Paris epa...@redhat.com Date: Fri, 03 Jun 2011 18:04:51 -0400 ... Signed-off-by: Eric Paris epa...@redhat.com Acked-by: Acked-by: H. Peter Anvin h...@zytor.com [for x86 portion] For sparc parts: Acked-by: David S. Miller da...@davemloft.net ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH -v2] Audit: push audit success and retcode into arch ptrace.h
Am Samstag 04 Juni 2011, 00:04:51 schrieb Eric Paris: The audit system previously expected arches calling to audit_syscall_exit to supply as arguments if the syscall was a success and what the return code was. Audit also provides a helper AUDITSC_RESULT which was supposed to simplify things by converting from negative retcodes to an audit internal magic value stating success or failure. This helper was wrong and could indicate that a valid pointer returned to userspace was a failed syscall. The fix is to fix the layering foolishness. We now pass audit_syscall_exit a struct pt_reg and it in turns calls back into arch code to collect the return value and to determine if the syscall was a success or failure. We also define a generic is_syscall_success() macro which determines success/failure based on if the value is -MAX_ERRNO. This works for arches like x86 which do not use a separate mechanism to indicate syscall failure. In arch/sh/kernel/ptrace_64.c I see that we were using regs[9] in the old audit code as the return value. But the ptrace_64.h code defined the macro regs_return_value() as regs[3]. I have no idea which one is correct, but this patch now uses the regs_return_value() function, so it now uses regs[3]. We make both the is_syscall_success() and regs_return_value() static inlines instead of macros. The reason is because the audit function must take a void* for the regs. (uml calls theirs struct uml_pt_regs instead of just struct pt_regs so audit_syscall_exit can't take a struct pt_regs). Since the audit function takes a void* we need to use static inlines to cast it back to the arch correct structure to dereference it. The other major change is that on some arches, like ia64, we change regs_return_value() to give us the negative value on syscall failure. The only other user of this macro, kretprobe_example.c, won't notice and it makes the value signed consistently for the audit functions across all archs. Signed-off-by: Eric Paris epa...@redhat.com Acked-by: Acked-by: H. Peter Anvin h...@zytor.com [for x86 portion] The UML part is now fine for me. :-) Acked-by: Richard Weinberger rich...@nod.at Thanks, //richard ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH -v2] Audit: push audit success and retcode into arch ptrace.h
The audit system previously expected arches calling to audit_syscall_exit to
supply as arguments if the syscall was a success and what the return code was.
Audit also provides a helper AUDITSC_RESULT which was supposed to simplify
things
by converting from negative retcodes to an audit internal magic value stating
success or failure. This helper was wrong and could indicate that a valid
pointer returned to userspace was a failed syscall. The fix is to fix the
layering foolishness. We now pass audit_syscall_exit a struct pt_reg and it
in turns calls back into arch code to collect the return value and to
determine if the syscall was a success or failure. We also define a generic
is_syscall_success() macro which determines success/failure based on if the
value is -MAX_ERRNO. This works for arches like x86 which do not use a
separate mechanism to indicate syscall failure.
In arch/sh/kernel/ptrace_64.c I see that we were using regs[9] in the old
audit code as the return value. But the ptrace_64.h code defined the macro
regs_return_value() as regs[3]. I have no idea which one is correct, but this
patch now uses the regs_return_value() function, so it now uses regs[3].
We make both the is_syscall_success() and regs_return_value() static inlines
instead of macros. The reason is because the audit function must take a void*
for the regs. (uml calls theirs struct uml_pt_regs instead of just struct
pt_regs so audit_syscall_exit can't take a struct pt_regs). Since the audit
function takes a void* we need to use static inlines to cast it back to the
arch correct structure to dereference it.
The other major change is that on some arches, like ia64, we change
regs_return_value() to give us the negative value on syscall failure. The
only other user of this macro, kretprobe_example.c, won't notice and it makes
the value signed consistently for the audit functions across all archs.
Signed-off-by: Eric Paris epa...@redhat.com
Acked-by: Acked-by: H. Peter Anvin h...@zytor.com [for x86 portion]
---
arch/ia64/include/asm/ptrace.h| 13 -
arch/ia64/kernel/ptrace.c |9 +
arch/microblaze/include/asm/ptrace.h |5 +
arch/microblaze/kernel/ptrace.c |3 +--
arch/mips/include/asm/ptrace.h| 14 +-
arch/mips/kernel/ptrace.c |4 +---
arch/powerpc/include/asm/ptrace.h | 10 +-
arch/powerpc/kernel/ptrace.c |4 +---
arch/s390/include/asm/ptrace.h|6 +-
arch/s390/kernel/ptrace.c |4 +---
arch/sh/include/asm/ptrace_32.h |5 -
arch/sh/include/asm/ptrace_64.h |5 -
arch/sh/kernel/ptrace_32.c|4 +---
arch/sh/kernel/ptrace_64.c|4 +---
arch/sparc/include/asm/ptrace.h | 10 +-
arch/sparc/kernel/ptrace_64.c | 11 +--
arch/um/kernel/ptrace.c |4 ++--
arch/um/sys-i386/shared/sysdep/ptrace.h |4
arch/um/sys-x86_64/shared/sysdep/ptrace.h |4
arch/x86/ia32/ia32entry.S | 10 +-
arch/x86/kernel/entry_32.S|8
arch/x86/kernel/entry_64.S| 10 +-
arch/x86/kernel/ptrace.c |3 +--
arch/x86/kernel/vm86_32.c |4 ++--
include/linux/audit.h | 22 ++
include/linux/ptrace.h| 10 ++
kernel/auditsc.c | 16
27 files changed, 132 insertions(+), 74 deletions(-)
diff --git a/arch/ia64/include/asm/ptrace.h b/arch/ia64/include/asm/ptrace.h
index 7ae9c3f..801ee1a 100644
--- a/arch/ia64/include/asm/ptrace.h
+++ b/arch/ia64/include/asm/ptrace.h
@@ -246,7 +246,18 @@ static inline unsigned long user_stack_pointer(struct
pt_regs *regs)
return regs-ar_bspstore;
}
-#define regs_return_value(regs) ((regs)-r8)
+static inline int is_syscall_success(struct pt_regs *regs)
+{
+ return regs-r10 != -1;
+}
+
+static inline long regs_return_value(struct pt_regs *regs)
+{
+ if (is_syscall_success(regs))
+ return regs-r8;
+ else
+ return -regs-r8;
+}
/* Conserve space in histogram by encoding slot bits in address
* bits 2 and 3 rather than bits 0 and 1.
diff --git a/arch/ia64/kernel/ptrace.c b/arch/ia64/kernel/ptrace.c
index 8848f43..2c15408 100644
--- a/arch/ia64/kernel/ptrace.c
+++ b/arch/ia64/kernel/ptrace.c
@@ -1268,14 +1268,7 @@ syscall_trace_leave (long arg0, long arg1, long arg2,
long arg3,
{
int step;
- if (unlikely(current-audit_context)) {
- int success = AUDITSC_RESULT(regs.r10);
- long result = regs.r8;
-
- if (success != AUDITSC_SUCCESS)
- result = -result;
- audit_syscall_exit(success, result);
- }
+
Re: [PATCH -v2] Audit: push audit success and retcode into arch ptrace.h
On Fri, Jun 3, 2011 at 3:04 PM, Eric Paris epa...@redhat.com wrote:
The other major change is that on some arches, like ia64, we change
regs_return_value() to give us the negative value on syscall failure. The
only other user of this macro, kretprobe_example.c, won't notice and it makes
the value signed consistently for the audit functions across all archs.
v2 builds and boots on ia64 now
Acked-by: Tony Luck tony.l...@intel.com
Signed-off-by: Eric Paris epa...@redhat.com
Acked-by: Acked-by: H. Peter Anvin h...@zytor.com [for x86 portion]
:-)
-Tony
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
