Messages in this thread | | | From | Eiichi Tsukata <> | Subject | Re: [PATCH] audit: use pid.is_auditd to make auditd_test_task() faster | Date | Mon, 17 Apr 2023 11:42:15 +0000 |
| |
> On Apr 14, 2023, at 23:44, Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Apr 13, 2023 at 11:14 PM 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 >> ========= >> >> 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@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 “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))
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.
Thanks Eiichi
| |