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

[Eiichi Tsukata]

> 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))

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