Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230523181624.19932-1-ivan@cloudflare.com> <CAHC9VhTtbbiwyOqiFfveWF6hV-Wb5cuhk0r8EdUi0kVb0v4P_Q@mail.gmail.com>
 <CABWYdi2A8ZfVJjmDpwiNhvYD8m-+PC5MsNRzofX7SXn2TTyY9Q@mail.gmail.com> <CAHC9VhQm9JEFozFMvNuBc_dx+XAqvJCY_Z8Dyf7q_RyDcNu=QA@mail.gmail.com>
In-Reply-To: <CAHC9VhQm9JEFozFMvNuBc_dx+XAqvJCY_Z8Dyf7q_RyDcNu=QA@mail.gmail.com>
From:   Ivan Babrou <ivan@cloudflare.com>
Date:   Wed, 24 May 2023 11:04:55 -0700
Message-ID: <CABWYdi3_zAVpeTRBou_Br-n6VXeM1xWTCSvu==QWdG4sd+nnnw@mail.gmail.com>
Subject: Re: [PATCH] audit: check syscall bitmap on entry to avoid extra work
To:     Paul Moore <paul@paul-moore.com>
Cc:     audit@vger.kernel.org, linux-kernel@vger.kernel.org,
        kernel-team@cloudflare.com, Eric Paris <eparis@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, May 23, 2023 at 7:03=E2=80=AFPM Paul Moore <paul@paul-moore.com> wr=
ote:
> > 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. The
next best thing I could think of is checking a trivial condition (int
equality + bit check) to bypass the work in the hot path, which is
what this patch does. I'm not even adding a new condition for this,
I'm using the existing context->dummy. It is also 100% transparent for
end users, which get the benefits as long as they don't have any rules
that target all syscalls.

It's not very useful to see futex() and stat() syscalls being audited
when we have no rules that target those. The processing of rules in
exit is already fast for a reasonable set of rules, but we don't have
to do it to begin with. List iteration is not going to be faster than
a bit check. For VFS related things we also have to collect paths
accessed during processing and it's just not useful when we know that
there is no way these are going to be used.

We started with a ruleset that was matching all syscalls and this was
very expensive. We reduced it to targeting specific syscalls, which
made it a lot cheaper, but it's still a very noticeable fraction of
overall CPU usage (the benchmark in the commit is the evidence of
that). Not enabling auditing on syscall entry is the next logical step
in making audit cheap enough to not feel guilty about using it in the
first place.