Re: [PATCH] audit: use pid.is_auditd to make auditd_test_task() faster

[Paul Moore]

On Mon, Apr 17, 2023 at 7:42 AM Eiichi Tsukata
<eiichi.tsukata@xxxxxxxxxxx> wrote:
> > On Apr 14, 2023, at 23:44, Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > On Thu, Apr 13, 2023 at 11:14 PM Eiichi Tsukata
> > <eiichi.tsukata@xxxxxxxxxxx> wrote:
> >>
> >> auditd_test_task() is a hot path of system call auditing. This patch
> >> introduces a new bit field "is_auditd" in pid struct which can be used
> >> for faster check of registered audit daemon.
> >>
> >> Benchmark
> >> =========
> >>
> >> Run the following command:
> >>
> >>  dd if=/dev/zero of=/dev/null bs=1 count=5M
> >>
> >> With rule:
> >>
> >>  -a never,exit -F arch=b64 -S uname
> >>
> >> Result:
> >>
> >>  Base line    : 2.572 sec
> >>  /w this patch: 2.412 sec (6.6% faster)
> >>
> >> Signed-off-by: Eiichi Tsukata <eiichi.tsukata@xxxxxxxxxxx>
> >> ---
> >> include/linux/pid.h |  4 ++++
> >> kernel/audit.c      | 22 ++--------------------
> >> kernel/audit.h      |  3 ++-
> >> kernel/pid.c        |  3 +++
> >> 4 files changed, 11 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/include/linux/pid.h b/include/linux/pid.h
> >> index 343abf22092e..5fe38e254c9a 100644
> >> --- a/include/linux/pid.h
> >> +++ b/include/linux/pid.h
> >> @@ -68,6 +68,10 @@ struct pid
> >>        wait_queue_head_t wait_pidfd;
> >>        struct rcu_head rcu;
> >>        struct upid numbers[1];
> >> +#ifdef CONFIG_AUDIT
> >> +       /* registered audit daemon tgid */
> >> +       unsigned is_auditd:1;
> >> +#endif
> >> };
> >
> > Thank you for the patch, but I don't think we want to add an audit
> > specific field to the pid struct at this time.
> >
>
> Hi Paul
>
> I agree “is_auditd” is too specific.
>
> How about having global “auditd_pid” struct pid pointer and let auditd_test_task() use it?
> I mean:
> #define auditd_test_task(tsk) (READ_ONCE(auditd_pid) == task_tgid(tsk))

At this point in time I prefer to keep the auditd pid in the
auditd_connection struct.

> By the way, it’s a bit different topic,  I may have found a race in usage of auditd_pid_vnr().
>  In AUDIT_SET handling, the variable auditd_pid is referenced outside of the spinlock so it can be changed while it’s referenced.
> So there is a TOCTOU race like this:
>
> CPU0                                        CPU1
> =====                                       =====
> auditd = auditd_pid_vnr()
>                                                   auditd = auditd_pid_vnr()
> if (auditd_pid) {…}
>                                                   if (auditd_pid) {…}
> auditd_set()
>                                                   auditd_set()
>
>
>
> If auditd_pid_vnr() returns 0, this case can lead to replacement of a healthy auditd, which seems to be prohibited judging from the code comment "/* replacing a healthy auditd is not allowed */“.
>
> Please correct me if I’m wrong.

Simultaneous AUDIT_SET operations are prevented by the
audit_cmd_mutex/audit_ctl_lock(), see audit_receive().

-- 
paul-moore.com