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: <20230414031325.82840-1-eiichi.tsukata@nutanix.com>
 <CAHC9VhSM9Pfe-HEHh0zGK473d=sWAEvoAxYVWVmsTos=EqqQOg@mail.gmail.com> <7FCAD214-5E79-4D07-A111-3657E91758B1@nutanix.com>
In-Reply-To: <7FCAD214-5E79-4D07-A111-3657E91758B1@nutanix.com>
From:   Paul Moore <paul@paul-moore.com>
Date:   Mon, 17 Apr 2023 16:27:39 -0400
Message-ID: <CAHC9VhTRXwrKkXMFJNiVgZbV=ZHExHa2VhBo6DKPEKZ8xcS=iw@mail.gmail.com>
Subject: Re: [PATCH] audit: use pid.is_auditd to make auditd_test_task() faster
To:     Eiichi Tsukata <eiichi.tsukata@nutanix.com>
Cc:     "eparis@redhat.com" <eparis@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "audit@vger.kernel.org" <audit@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Apr 17, 2023 at 7:42=E2=80=AFAM Eiichi Tsukata
<eiichi.tsukata@nutanix.com> wrote:
> > On Apr 14, 2023, at 23:44, Paul Moore <paul@paul-moore.com> wrote:
> > On Thu, Apr 13, 2023 at 11:14=E2=80=AFPM Eiichi Tsukata
> > <eiichi.tsukata@nutanix.com> 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
> >> =3D=3D=3D=3D=3D=3D=3D=3D=3D
> >>
> >> Run the following command:
> >>
> >>  dd if=3D/dev/zero of=3D/dev/null bs=3D1 count=3D5M
> >>
> >> With rule:
> >>
> >>  -a never,exit -F arch=3Db64 -S uname
> >>
> >> Result:
> >>
> >>  Base line    : 2.572 sec
> >>  /w this patch: 2.412 sec (6.6% faster)
> >>
> >> Signed-off-by: Eiichi Tsukata <eiichi.tsukata@nutanix.com>
> >> ---
> >> 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 =E2=80=9Cis_auditd=E2=80=9D is too specific.
>
> How about having global =E2=80=9Cauditd_pid=E2=80=9D struct pid pointer a=
nd let auditd_test_task() use it?
> I mean:
> #define auditd_test_task(tsk) (READ_ONCE(auditd_pid) =3D=3D task_tgid(tsk=
))

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

> By the way, it=E2=80=99s 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=E2=80=99s referenced.
> So there is a TOCTOU race like this:
>
> CPU0                                        CPU1
> =3D=3D=3D=3D=3D                                       =3D=3D=3D=3D=3D
> auditd =3D auditd_pid_vnr()
>                                                   auditd =3D auditd_pid_v=
nr()
> if (auditd_pid) {=E2=80=A6}
>                                                   if (auditd_pid) {=E2=80=
=A6}
> auditd_set()
>                                                   auditd_set()
>
>
>
> If auditd_pid_vnr() returns 0, this case can lead to replacement of a hea=
lthy auditd, which seems to be prohibited judging from the code comment "/*=
 replacing a healthy auditd is not allowed */=E2=80=9C.
>
> Please correct me if I=E2=80=99m wrong.

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

--=20
paul-moore.com