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

[Eiichi Tsukata]

> On Apr 18, 2023, at 5:27, Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> 
> 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.

OK, but let me try out it as it should look pretty simple.
Will post v2 later.

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

Thanks, I missed that. Understood.

Eiichi