Re: [PATCH v2.1] audit: Only use the syscall slowpath when syscall audit rules exist
On Mon, 2014-02-03 at 11:11 -0800, Andy Lutomirski wrote:
+void audit_inc_n_rules()
+{
+ struct task_struct *p, *g;
+ unsigned long flags;
+
+ read_lock_irqsave(tasklist_lock, flags);
+ if (audit_n_rules++ == 0) {
I know it's right, but it's too clever for me :) If we do end up
adding something like this Can we just do:
if (!audit_n_rules) {}
audit_n_rules++
I like dumb code :)
+ do_each_thread(g, p) {
+ if (p-audit_context)
+ set_tsk_thread_flag(p, TIF_SYSCALL_AUDIT);
+ } while_each_thread(g, p);
+ }
+ read_unlock_irqrestore(tasklist_lock, flags);
+}
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2.1] audit: Only use the syscall slowpath when syscall audit rules exist
On 02/03, Andy Lutomirski wrote:
+void audit_inc_n_rules()
+{
+ struct task_struct *p, *g;
+ unsigned long flags;
+
+ read_lock_irqsave(tasklist_lock, flags);
Confused... read_lock(tasklist) doesn't need to disable irqs.
(ftrace does this for no reason too, perhaps I should resend the patch)
+ if (audit_n_rules++ == 0) {
probably this can be done outside of read_lock?
+ do_each_thread(g, p) {
for_each_process_thread ;) do_each_thread will die, I hope.
+void audit_dec_n_rules()
+{
+ struct task_struct *p, *g;
+ unsigned long flags;
+
+ read_lock_irqsave(tasklist_lock, flags);
+
+ --audit_n_rules;
+ BUG_ON(audit_n_rules 0);
+
+ if (audit_n_rules == 0) {
+ do_each_thread(g, p) {
+ clear_tsk_thread_flag(p, TIF_SYSCALL_AUDIT);
+ } while_each_thread(g, p);
+ }
The same, and...
On a second thought it seems that audit_dec_n_rules() has a problem.
Note the BUG_ON(context-in_syscall) in __audit_syscall_entry().
Suppose that audit_dec_n_rules() clears TIF_SYSCALL_AUDIT when a task
runs a syscall. In this case (afaics) __audit_syscall_exit() won't be
called. The next audit_inc_n_rules() can set TIF_SYSCALL_AUDIT and
trigger another __audit_syscall_entry() which will hit this BUG_ON().
And in general it doesn't look safe although I know almost nothing
about audit. I mean, currently __audit_syscall_entry() or
__audit_log_bprm_fcaps() assume that __audit_syscall_exit() or
__audit_free() will cleanup -audit_context, perhaps we should not
break the rules?
Once again, I do not pretend I understand this code, this is the
question, not the comment.
But if I am right, then TIF_SYSCALL_AUDIT should be cleared in
__audit_syscall_exit() as you suggested before.
Oleg.
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2.1] audit: Only use the syscall slowpath when syscall audit rules exist
On Tue, Feb 4, 2014 at 8:50 AM, Oleg Nesterov o...@redhat.com wrote:
On 02/03, Andy Lutomirski wrote:
+void audit_inc_n_rules()
+{
+ struct task_struct *p, *g;
+ unsigned long flags;
+
+ read_lock_irqsave(tasklist_lock, flags);
Confused... read_lock(tasklist) doesn't need to disable irqs.
(ftrace does this for no reason too, perhaps I should resend the patch)
Is this because there are no interrupt handlers that write_lock(tasklist)?
+ if (audit_n_rules++ == 0) {
probably this can be done outside of read_lock?
I don't think so. I'm cheating and using the tasklist_lock to prevent
audit_sync_flags from racing against this increment, so this needs to
be inside the lock. I'll add a comment.
+ do_each_thread(g, p) {
for_each_process_thread ;) do_each_thread will die, I hope.
Sorry, forgot to mention: where is this mythical
for_each_process_thread? Either it only exists in a kernel version
I'm not looking at, my grepping skills aren't up to the task, or you
just hate do_each_thread so much that you imagined up an alternative
:)
+void audit_dec_n_rules()
+{
+ struct task_struct *p, *g;
+ unsigned long flags;
+
+ read_lock_irqsave(tasklist_lock, flags);
+
+ --audit_n_rules;
+ BUG_ON(audit_n_rules 0);
+
+ if (audit_n_rules == 0) {
+ do_each_thread(g, p) {
+ clear_tsk_thread_flag(p, TIF_SYSCALL_AUDIT);
+ } while_each_thread(g, p);
+ }
The same, and...
On a second thought it seems that audit_dec_n_rules() has a problem.
Note the BUG_ON(context-in_syscall) in __audit_syscall_entry().
Suppose that audit_dec_n_rules() clears TIF_SYSCALL_AUDIT when a task
runs a syscall. In this case (afaics) __audit_syscall_exit() won't be
called. The next audit_inc_n_rules() can set TIF_SYSCALL_AUDIT and
trigger another __audit_syscall_entry() which will hit this BUG_ON().
Egads!
And in general it doesn't look safe although I know almost nothing
about audit. I mean, currently __audit_syscall_entry() or
__audit_log_bprm_fcaps() assume that __audit_syscall_exit() or
__audit_free() will cleanup -audit_context, perhaps we should not
break the rules?
Once again, I do not pretend I understand this code, this is the
question, not the comment.
But if I am right, then TIF_SYSCALL_AUDIT should be cleared in
__audit_syscall_exit() as you suggested before.
I think I'll wait for Eric to chime in. I suspect that the real
solution is to simplify all this stuff by relying on the fact that the
syscall nr and args are saved by the (fast path and slow path) entry
code, so the audit entry hook may be entirely unnecessary. Or maybe a
new TIF flag would be needed to make it work.
Anyway, *grumble*. My only real interest in this stuff is to get rid
of the overhead. I don't actually want syscall auditing on any of my
boxes (in contrast to avc auditing, which is useful but (mostly)
independent).
--Andy
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2.1] audit: Only use the syscall slowpath when syscall audit rules exist
On 02/04, Andy Lutomirski wrote:
On Tue, Feb 4, 2014 at 8:50 AM, Oleg Nesterov o...@redhat.com wrote:
On 02/03, Andy Lutomirski wrote:
+void audit_inc_n_rules()
+{
+ struct task_struct *p, *g;
+ unsigned long flags;
+
+ read_lock_irqsave(tasklist_lock, flags);
Confused... read_lock(tasklist) doesn't need to disable irqs.
(ftrace does this for no reason too, perhaps I should resend the patch)
Is this because there are no interrupt handlers that write_lock(tasklist)?
Yes.
+ if (audit_n_rules++ == 0) {
probably this can be done outside of read_lock?
I don't think so. I'm cheating and using the tasklist_lock to prevent
audit_sync_flags
Ah, yes, you are right.
+ do_each_thread(g, p) {
for_each_process_thread ;) do_each_thread will die, I hope.
Sorry, forgot to mention: where is this mythical
for_each_process_thread?
In Linus's tree, please see 0c740d0afc3bff.
or you
just hate do_each_thread so much that you imagined up an alternative
:)
sort of ;)
I think I'll wait for Eric to chime in. I suspect that the real
solution is to simplify all this stuff by relying on the fact that the
syscall nr and args are saved by the (fast path and slow path) entry
code, so the audit entry hook may be entirely unnecessary.
Perhaps... but even in this case we need to do something with, say,
__audit_log_bprm_fcaps().
At least this list should not grow indefinitely if the task skips
__audit_syscall_exit(). Although at first glance this can be probably
cleanuped too.
Oleg.
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2.1] audit: Only use the syscall slowpath when syscall audit rules exist
On Tue, Feb 4, 2014 at 11:11 AM, Oleg Nesterov o...@redhat.com wrote: On 02/04, Andy Lutomirski wrote: On Tue, Feb 4, 2014 at 8:50 AM, Oleg Nesterov o...@redhat.com wrote: On 02/03, Andy Lutomirski wrote: Sorry, forgot to mention: where is this mythical for_each_process_thread? In Linus's tree, please see 0c740d0afc3bff. or you just hate do_each_thread so much that you imagined up an alternative :) sort of ;) Aha -- it probably got merged just after I pulled Linus' tree and looked for it. Or something. I think I'll wait for Eric to chime in. I suspect that the real solution is to simplify all this stuff by relying on the fact that the syscall nr and args are saved by the (fast path and slow path) entry code, so the audit entry hook may be entirely unnecessary. Perhaps... but even in this case we need to do something with, say, __audit_log_bprm_fcaps(). At least this list should not grow indefinitely if the task skips __audit_syscall_exit(). Although at first glance this can be probably cleanuped too. OK, here's a thought: let's change the semantics of TIF_SYSCALL_AUDIT. New semantics: TIF_SYSCALL_AUDIT is set if (the task is eligible for syscall auditing and n_rules != 0 *or* something has started writing an audit record). TIF_SYSCALL_AUDIT is *never* cleared by anything other than copy_process or __audit_syscall_exit. This means that, if there's a pending audit record, then TIF_SYSCALL_AUDIT will be set. That, in turn, means that __audit_syscall_exit will be called, which avoids the BUGs you pointed out. Now we get rid of __audit_syscall_entry. (This speeds up even the auditing-is-on case.) Instead we have __audit_start_record, which does more or less the same thing, except that (a) it doesn't BUG if in_syscall and (b) it *sets* TIF_SYSCALL_AUDIT. This relies on the fact that syscall_get_nr and syscall_get_arguments are reliable on x86_64. I suspect that they're reliable everywhere else, too. The idea is that there's nothing wrong with calling __audit_start_record more than once. (Maybe it should be called __audit_record_this_syscall.) To finish the job, we change __audit_syscall_exit to clear TIF_SYSCALL_AUDIT if n_rules==0. inc_n_rules can set TIF_SYSCALL_AUDIT (without even needing to worry about races, I think). dec_n_rules doesn't need to do anything special. Benefits: - Removing the syscall entry hook speeds everyone up. Even silly people who use exit rules :) - There's no need to think about mismatched entry/exit hook calls, since there is no entry hook. - The same mechanism could be reused for non-audit purposes. Any code could say, at any point hey, this syscall is interesting. let's record it. Disadvantages: - Need to check other architectures to make sure that syscall_get_xyz works reliably for fast path syscalls. - For the full performance boost, all architectures need to avoid checking TIF_SYSCALL_AUDIT in the entry path. I prefer not to mess with non-x86 assembly, so I won't do that part, since it's not required for correctness. Eric, any thoughts? --Andy -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
