lkml.org 
[lkml]   [2023]   [Apr]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] audit: use pid.is_auditd to make auditd_test_task() faster
Date


> 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
\
 
 \ /
  Last update: 2023-04-17 15:34    [W:0.103 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site