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>
In-Reply-To: <20230523181624.19932-1-ivan@cloudflare.com>
From:   Paul Moore <paul@paul-moore.com>
Date:   Tue, 23 May 2023 15:59:25 -0400
Message-ID: <CAHC9VhTtbbiwyOqiFfveWF6hV-Wb5cuhk0r8EdUi0kVb0v4P_Q@mail.gmail.com>
Subject: Re: [PATCH] audit: check syscall bitmap on entry to avoid extra work
To:     Ivan Babrou <ivan@cloudflare.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 2:16=E2=80=AFPM Ivan Babrou <ivan@cloudflare.com> w=
rote:
> Currently audit subsystem arms itself as long as there are rules present,
> which means that on every syscall exit all rules are evaluated, even
> if they don't match the syscall to begin with. For setups where
> there are no rules that can match any syscall, this means that
> the CPU price needs to be paid when it's not necessary.
>
> This patch introduces a bitmap for syscalls that is maintained
> when rules are inserted and removed. For every syscall we maintain
> a bit indicating whether it needs to be audited at all, which is then
> checked at syscall entry. If the are no rules matching a syscall,
> extra cost of checking all the rules is avoided.
>
> Consider the following set of 10 audit rules as a benchmark:
>
>     -a always,exit -F arch=3Db64 -S unlinkat,linkat,renameat,openat,renam=
eat2 -F perm=3Dwa -F dir=3D/tmp/audit-bench/0 -F key=3DBENCH0
>     -a always,exit -F arch=3Db64 -S unlinkat,linkat,renameat,openat,renam=
eat2 -F perm=3Dwa -F dir=3D/tmp/audit-bench/1 -F key=3DBENCH1
>     -a always,exit -F arch=3Db64 -S unlinkat,linkat,renameat,openat,renam=
eat2 -F perm=3Dwa -F dir=3D/tmp/audit-bench/2 -F key=3DBENCH2
>     -a always,exit -F arch=3Db64 -S unlinkat,linkat,renameat,openat,renam=
eat2 -F perm=3Dwa -F dir=3D/tmp/audit-bench/3 -F key=3DBENCH3
>     -a always,exit -F arch=3Db64 -S unlinkat,linkat,renameat,openat,renam=
eat2 -F perm=3Dwa -F dir=3D/tmp/audit-bench/4 -F key=3DBENCH4
>     -a always,exit -F arch=3Db64 -S unlinkat,linkat,renameat,openat,renam=
eat2 -F perm=3Dwa -F dir=3D/tmp/audit-bench/5 -F key=3DBENCH5
>     -a always,exit -F arch=3Db64 -S unlinkat,linkat,renameat,openat,renam=
eat2 -F perm=3Dwa -F dir=3D/tmp/audit-bench/6 -F key=3DBENCH6
>     -a always,exit -F arch=3Db64 -S unlinkat,linkat,renameat,openat,renam=
eat2 -F perm=3Dwa -F dir=3D/tmp/audit-bench/7 -F key=3DBENCH7
>     -a always,exit -F arch=3Db64 -S unlinkat,linkat,renameat,openat,renam=
eat2 -F perm=3Dwa -F dir=3D/tmp/audit-bench/8 -F key=3DBENCH8
>     -a always,exit -F arch=3Db64 -S unlinkat,linkat,renameat,openat,renam=
eat2 -F perm=3Dwa -F dir=3D/tmp/audit-bench/9 -F key=3DBENCH9
>
> We can use the following benchmark to run unrelated syscalls:
>
>     #include <sys/stat.h>
>     #include <unistd.h>
>     #include <stdio.h>
>
>     #define GETPID_COUNT 100 * 1000
>     #define STAT_COUNT 100 * 1000
>
>     pid_t bench_getpid()
>     {
>         pid_t pid;
>
>         for (int i =3D 0; i < GETPID_COUNT; i++)
>         {
>             pid =3D getpid();
>         }
>
>         return pid;
>     }
>
>     struct stat bench_stat()
>     {
>         struct stat statbuf;
>
>         for (int i =3D 0; i < STAT_COUNT; i++)
>         {
>             stat("/etc/passwd", &statbuf);
>         }
>
>         return statbuf;
>     }
>
>     int main()
>     {
>         pid_t pid =3D bench_getpid();
>         struct stat statbuf =3D bench_stat();
>
>         printf("pid =3D %d, size =3D %d\n", pid, statbuf.st_size);
>     }
>
> Here we run 100k `getpid()` calls and 100k `stat()` calls, which are not
> covered by any of the audit rules installed on the system.
>
> When running without any rules present, but with auditd running, flamegra=
phs
> show ~5% of CPU time spent in audit_* code. If we install the rules menti=
oned
> above, this number jumps to ~24%. With this patch applied, the number is =
once
> again down to 5%, which is what one would expect.

Before seriously considering something like this, I would really like
to see some time put into profiling the original overhead and some
designs on how that could be improved.  Without that, patches like
this look like drive-by band-aids which have already caused enough
headaches for audit maintenance.

> There's extra cost of maintaining the bitmap when rules are changed,
> but it's negligible compared to CPU savings from cheaper syscalls.
>
> Signed-off-by: Ivan Babrou <ivan@cloudflare.com>
> ---
>  include/linux/audit.h | 21 +++++++++++++++++++++
>  kernel/auditfilter.c  | 32 ++++++++++++++++++++++++++++----
>  kernel/auditsc.c      | 27 +++++++++++----------------
>  3 files changed, 60 insertions(+), 20 deletions(-)

--=20
paul-moore.com