Re: [PATCH] audit: check syscall bitmap on entry to avoid extra work

[Ignat Korchagin]

On Thu, May 25, 2023 at 3:15 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>
> On Wed, May 24, 2023 at 2:05 PM Ivan Babrou <ivan@xxxxxxxxxxxxxx> wrote:
> >
> > On Tue, May 23, 2023 at 7:03 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > > > Could you elaborate on what exactly you would like to see added? It's
> > > > not clear to me what is missing.
> > >
> > > I should have been more clear, let me try again ...
> > >
> > > From my perspective, this patch adds code and complexity to deal with
> > > the performance impact of auditing.  In some cases that is the right
> > > thing to do, but I would much rather see a more in-depth analysis of
> > > where the audit hot spots are in this benchmark, and some thoughts on
> > > how we might improve that.  In other words, don't just add additional
> > > processing to bypass (slower, more involved) processing; look at the
> > > processing that is currently being done and see if you can find a way
> > > to make it faster.  It will likely take longer, but the results will
> > > be much more useful.
> >
> > The fastest way to do something is to not do it to begin with.
>
> While you are not wrong, I believe you and I are focusing on different
> things.  From my perspective, you appear primarily concerned with
> improving performance by reducing the overhead of auditing.  I too am
> interested in reducing the audit overhead, but I also place a very
> high value on maintainable code, perhaps more than performance simply
> because the current audit code quality is so very poor.
> Unfortunately, the patch you posted appears to me as yet another
> bolt-on performance tweak that doesn't make an attempt at analyzing
> the current hot spots of syscall auditing, and ideally offering
> solutions.  Perhaps ultimately this approach is the only sane thing
> that can be done, but I'd like to see some analysis first of the
> syscall auditing path.

Ivan is out of office, but I would like to keep the discussion going.
We do understand your position and we're actually doing a project
right now to investigate audit performance better and this patch is
only the first thing which came up. Perhaps, we would have some other
improvements in the future.

But the way I see it - the audit subsystem performance and the way how
that subsystem plugs into the rest of the kernel code are two somewhat
independent things with the patch proposed here addressing the latter
(with full understanding that the former might be improved as well). I
think the fundamental issue we're trying to address here is that the
audit subsystem plugs into the kernel in a way, which is different
from conceptually similar systems, like netfilter, LSM,
kprobes/tracepoints etc, thus potentially breaking user expectations.
For example, when I use netfilter or LSMs - I see it as a system of
hooks: my code/rules/inspection plugs into some hooks of interest and
I fully understand that the kernel codepaths, where these hooks are
located, might be affected due to additional processing attached. Thus
I can adjust my code/rules/inspection to plug into only the minimum
required hooks to get the visibility or the effect I need without too
much affecting system performance.

So I (and most of us here really) had the same expectation from audit:
we thought that when you have a rule, which "monitors" a certain
system call, only that system call performance would be affected. IOW
we thought that system calls are a kind of a hook for audit rules and
it was a surprise for us to learn that when having a rule, which
monitors execve() for example, we can impact other unrelated system
calls in totally different workloads. I think this behaviour
fundamentally limits the ability of the audit users to optimise their
rules to monitor/collect only the minimum required information,
because as long as they need to monitor at least one system call -
they effectively monitor all of them. I do strongly suspect that most
Linux users would have a similar (mis-)expectation from the audit
subsystem, hence I think this patch will align the audit subsystem
better with the expectations from users.

Ignat