Re: [PATCH] audit: speedup for syscalls when auditing is disabled

[Michael Neuling <mikey@neuling.org>]

> > On reflection, we might have a bug in audit_alloc though.  Currently we
> > have this:
> > 
> >   int audit_alloc(struct task_struct *tsk)
> >   {
> > 	  <snip>
> > 	  state = audit_filter_task(tsk, &key);
> > 	  if (likely(state == AUDIT_DISABLED))
> > 		  return 0;
> > 
> > 	  <snip>
> > 	  set_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT);
> > 	  return 0;
> >   }
> > 
> > This gets called on fork.  If we have "task,never" rule, we hit this
> > state == AUDIT_DISABLED path, return immediately and the tasks
> > TIF_SYSCALL_AUDIT flags doesn't get set.  On powerpc, we check
> > TIF_SYSCALL_AUDIT in asm on syscall entry to fast path not calling the
> > syscall audit code.
> 
> I'm guessing it actually bypasses audit if the flag is not set?  

Correct.

> So we might have a bug, but i'd be surprised since I think we tested
> audit on powerpc....

So on powerpc we have this in entry_64.S

#define _TIF_SYSCALL_AUDIT	(1<<TIF_SYSCALL_AUDIT)
#define _TIF_SYSCALL_T_OR_A (_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT|_TIF_SECCOMP)

	andi.	r11,r10,_TIF_SYSCALL_T_OR_A
	bne-	syscall_dotrace

and there is similar code in x86 in entry_64.S

#ifdef CONFIG_AUDITSYSCALL
	bt $TIF_SYSCALL_AUDIT,%edx
	jc sysret_audit
#endif

So I think other archs are broken too.

I think the bug is in the generic code though.  Shouldn't syscall
auditing be enabled independent of task fork/exec auditing?

> > This seems wrong to me as a "never" _task_ audit rule shouldn't effect
> > _syscall_ auditing?  Is there some interaction between task and syscall
> > auditing that I'm missing?
> 
> There are 3 states for a given task, I don't remember the names off the
> top of my head, so I'll guess with: on, off, build.  'Build' is the
> state most processes usually live in.  In this state we collect audit
> information about the task during the whole syscall and then we might
> (likely) throw that information away at syscall exit.
> 
> Some types of audit rule, which alter this state, can be checked at
> either 'entry' or 'exit' (first rule wins) At syscall entry we only have
> enough information (questionable if we even have enough information at
> all but that's a different question) to filter based on the task.  You
> can create rules that will audit all tasks, or in your case will
> explicitly disable auditing for all tasks.

So does this mean that an "auditctl -a task,never" _should_ disable
syscall auding?  I'm not 100% clear on this still.  

> Normally a process would be in the default 'build' state after syscall
> entry, we will collect information about the syscall, and then we will
> check syscall rules at exit.  Once you explicitly say 'I do not want any
> audit messages for this task' you are in 'off' instead of 'build.'

Ok

> > > I wonder if you could get much back, in terms of performance, by moving
> > > the
> > >          context->dummy = !audit_n_rules;
> > > line to the top and just returning if context->dummy == 1;
> > 
> > We get 668.09 cycles with this optimisation, so it comes down a bit, but
> > no where near if the auditing is disabled altogether.
> 
> Clean that patch up and send it.  Sounds like a win no matter what else
> we do.

Ok, sending in separate email.

> > Like I said above, powerpc has a fast path in asm on system call entry
> > to check the thread_info flags for if syscall auditing is disabled.  If
> > it's disabled, we don't call the audit code, hence why it's very fast in
> > this case.
> 
> Here's a new idea to think about with obvious tradeoffs.  What do you
> think about doing a little bit of assembly rejiggering?
> 
> Add a new spot in the assembly which will call a function which will
> check if audit_n_rules > 0 and if so will set TIF_SYSCALL_AUDIT and if
> not will clear TIF_SYSCALL_AUDIT?  It might make things slightly worse
> on systems which explictly disable audit and the flag would always be
> clear on every task (like you did with the explicit rule) but I'm
> guessing might be a win on systems with no rules which are wasting time
> on the audit slow path.....

This sounds good to me except for one thing...

If we set TIF_SYSCALL_AUDIT when audit_n_rules > 0, it'll change the
functionality from what we have now in the "auditctl -a task,never"
case.  Currently in this case, TIF_SYSCALL_AUDIT will not be set and
syscalls won't be audited.  I think this is a bug (as discussed above),
but I wanted to point it out anyway.

Anyway, I'll take a stab at this in a bit.

Regards,
Mikey