[PATCH v2 0/2] audit: syscall audit optimization (> +6% faster)

[PATCH v2 0/2] audit: syscall audit optimization (> +6% faster)

[Eiichi Tsukata]

This patchset is an optimization of auditd_test_task() function.
As is described in Patch 1, it improves the performance of syscall
auditing.

Benchmarks
==========

Run the following micro benchmarks:

  (1) dd:
    dd if=/dev/zero of=/dev/null bs=1 count=5M

  (2) UnixBench syscall:
    ./Run syscall -i 10 -c 1

With rule:

  -a never,exit -F arch=b64 -S uname

Results:

  (1) dd
    Base line    : 2.572 sec
    /w this patch: 2.418 sec (6.3% faster)

  (2) UnixBench syscall Index Score
    Base line    : 860
    /w this patch: 953 (10.8% faster)


This patchset consists of the following parts:

  Patch 1: add global auditd_pid to make auditd_test_task() faster
  Patch 2: cleanup: replace auditd_conn.pid with auditd_pid

v1 -> v2:
  - Use global auditd_pid intead of pid.is_auditd
  - Add UnixBench syscall benchmark

v1: https://lore.kernel.org/audit/20230414031325.82840-1-eiichi.tsukata@nutanix.com/

Eiichi Tsukata (2):
  audit: add global auditd_pid to make auditd_test_task() faster
  audit: replace auditd_conn.pid with auditd_pid

 kernel/audit.c | 61 +++++++++++++++++---------------------------------
 kernel/audit.h |  4 +++-
 2 files changed, 24 insertions(+), 41 deletions(-)

-- 
2.39.2

[PATCH v2 1/2] audit: add global auditd_pid to make auditd_test_task() faster

[Eiichi Tsukata]

auditd_test_task() is a hot path of system call auditing. This patch
introduces a global auditd_pid pid struct which can be used for faster
check of registered audit daemon.

Benchmarks
==========

Run the following micro benchmarks:

  (1) dd:
    dd if=/dev/zero of=/dev/null bs=1 count=5M

  (2) UnixBench syscall:
    ./Run syscall -i 10 -c 1

With rule:

  -a never,exit -F arch=b64 -S uname

Results:

  (1) dd
    Base line    : 2.572 sec
    /w this patch: 2.418 sec (6.3% faster)

  (2) UnixBench syscall Index Score
    Base line    : 860
    /w this patch: 953 (10.8% faster)

Signed-off-by: Eiichi Tsukata <eiichi.tsukata@nutanix.com>
---
 kernel/audit.c | 39 +++++++++++----------------------------
 kernel/audit.h |  4 +++-
 2 files changed, 14 insertions(+), 29 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 9bc0b0301198..9426980368e4 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -71,6 +71,7 @@ static int	audit_initialized = AUDIT_UNINITIALIZED;
 
 u32		audit_enabled = AUDIT_OFF;
 bool		audit_ever_enabled = !!AUDIT_OFF;
+struct pid      *auditd_pid;
 
 EXPORT_SYMBOL_GPL(audit_enabled);
 
@@ -208,26 +209,6 @@ struct audit_reply {
 	struct sk_buff *skb;
 };
 
-/**
- * auditd_test_task - Check to see if a given task is an audit daemon
- * @task: the task to check
- *
- * Description:
- * Return 1 if the task is a registered audit daemon, 0 otherwise.
- */
-int auditd_test_task(struct task_struct *task)
-{
-	int rc;
-	struct auditd_connection *ac;
-
-	rcu_read_lock();
-	ac = rcu_dereference(auditd_conn);
-	rc = (ac && ac->pid == task_tgid(task) ? 1 : 0);
-	rcu_read_unlock();
-
-	return rc;
-}
-
 /**
  * audit_ctl_lock - Take the audit control lock
  */
@@ -512,6 +493,7 @@ static int auditd_set(struct pid *pid, u32 portid, struct net *net)
 	ac_old = rcu_dereference_protected(auditd_conn,
 					   lockdep_is_held(&auditd_conn_lock));
 	rcu_assign_pointer(auditd_conn, ac_new);
+	WRITE_ONCE(auditd_pid, ac_new->pid);
 	spin_unlock_irqrestore(&auditd_conn_lock, flags);
 
 	if (ac_old)
@@ -652,6 +634,7 @@ static void auditd_reset(const struct auditd_connection *ac)
 		return;
 	}
 	rcu_assign_pointer(auditd_conn, NULL);
+	WRITE_ONCE(auditd_pid, NULL);
 	spin_unlock_irqrestore(&auditd_conn_lock, flags);
 
 	if (ac_old)
@@ -1263,7 +1246,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 			 *       run auditd from the initial pid namespace, but
 			 *       something to keep in mind if this changes */
 			pid_t new_pid = s.pid;
-			pid_t auditd_pid;
+			pid_t auditd_pid_nr;
 			struct pid *req_pid = task_tgid(current);
 
 			/* Sanity check - PID values must match. Setting
@@ -1274,18 +1257,18 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 			/* test the auditd connection */
 			audit_replace(req_pid);
 
-			auditd_pid = auditd_pid_vnr();
-			if (auditd_pid) {
+			auditd_pid_nr = auditd_pid_vnr();
+			if (auditd_pid_nr) {
 				/* replacing a healthy auditd is not allowed */
 				if (new_pid) {
 					audit_log_config_change("audit_pid",
-							new_pid, auditd_pid, 0);
+							new_pid, auditd_pid_nr, 0);
 					return -EEXIST;
 				}
 				/* only current auditd can unregister itself */
-				if (pid_vnr(req_pid) != auditd_pid) {
+				if (pid_vnr(req_pid) != auditd_pid_nr) {
 					audit_log_config_change("audit_pid",
-							new_pid, auditd_pid, 0);
+							new_pid, auditd_pid_nr, 0);
 					return -EACCES;
 				}
 			}
@@ -1298,7 +1281,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 				if (audit_enabled != AUDIT_OFF)
 					audit_log_config_change("audit_pid",
 								new_pid,
-								auditd_pid,
+								auditd_pid_nr,
 								err ? 0 : 1);
 				if (err)
 					return err;
@@ -1309,7 +1292,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 				if (audit_enabled != AUDIT_OFF)
 					audit_log_config_change("audit_pid",
 								new_pid,
-								auditd_pid, 1);
+								auditd_pid_nr, 1);
 
 				/* unregister the auditd connection */
 				auditd_reset(NULL);
diff --git a/kernel/audit.h b/kernel/audit.h
index c57b008b9914..93074c2d4346 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -214,7 +214,9 @@ extern bool audit_ever_enabled;
 
 extern void audit_log_session_info(struct audit_buffer *ab);
 
-extern int auditd_test_task(struct task_struct *task);
+extern struct pid *auditd_pid;
+/* Check to see if a given task is an audit daemon */
+#define auditd_test_task(tsk) (READ_ONCE(auditd_pid) == task_tgid(tsk))
 
 #define AUDIT_INODE_BUCKETS	32
 extern struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
-- 
2.39.2

[PATCH v2 2/2] audit: replace auditd_conn.pid with auditd_pid

[Eiichi Tsukata]

auditd_conn.pid is redundant. Replace it with auditd_pid.

Signed-off-by: Eiichi Tsukata <eiichi.tsukata@nutanix.com>
---
 kernel/audit.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 9426980368e4..72a7397eaa89 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -94,7 +94,6 @@ struct audit_net {
 
 /**
  * struct auditd_connection - kernel/auditd connection state
- * @pid: auditd PID
  * @portid: netlink portid
  * @net: the associated network namespace
  * @rcu: RCU head
@@ -104,7 +103,6 @@ struct audit_net {
  * or the associated spinlock for writing.
  */
 struct auditd_connection {
-	struct pid *pid;
 	u32 portid;
 	struct net *net;
 	struct rcu_head rcu;
@@ -248,15 +246,11 @@ static bool audit_ctl_owner_current(void)
 static pid_t auditd_pid_vnr(void)
 {
 	pid_t pid;
-	const struct auditd_connection *ac;
+	unsigned long flags;
 
-	rcu_read_lock();
-	ac = rcu_dereference(auditd_conn);
-	if (!ac || !ac->pid)
-		pid = 0;
-	else
-		pid = pid_vnr(ac->pid);
-	rcu_read_unlock();
+	spin_lock_irqsave(&auditd_conn_lock, flags);
+	pid = auditd_pid ? pid_vnr(auditd_pid) : 0;
+	spin_unlock_irqrestore(&auditd_conn_lock, flags);
 
 	return pid;
 }
@@ -459,7 +453,6 @@ static void auditd_conn_free(struct rcu_head *rcu)
 	struct auditd_connection *ac;
 
 	ac = container_of(rcu, struct auditd_connection, rcu);
-	put_pid(ac->pid);
 	put_net(ac->net);
 	kfree(ac);
 }
@@ -478,6 +471,7 @@ static int auditd_set(struct pid *pid, u32 portid, struct net *net)
 {
 	unsigned long flags;
 	struct auditd_connection *ac_old, *ac_new;
+	struct pid *auditd_pid_old;
 
 	if (!pid || !net)
 		return -EINVAL;
@@ -485,7 +479,6 @@ static int auditd_set(struct pid *pid, u32 portid, struct net *net)
 	ac_new = kzalloc(sizeof(*ac_new), GFP_KERNEL);
 	if (!ac_new)
 		return -ENOMEM;
-	ac_new->pid = get_pid(pid);
 	ac_new->portid = portid;
 	ac_new->net = get_net(net);
 
@@ -493,9 +486,11 @@ static int auditd_set(struct pid *pid, u32 portid, struct net *net)
 	ac_old = rcu_dereference_protected(auditd_conn,
 					   lockdep_is_held(&auditd_conn_lock));
 	rcu_assign_pointer(auditd_conn, ac_new);
-	WRITE_ONCE(auditd_pid, ac_new->pid);
+	auditd_pid_old = auditd_pid;
+	WRITE_ONCE(auditd_pid, get_pid(pid));
 	spin_unlock_irqrestore(&auditd_conn_lock, flags);
 
+	put_pid(auditd_pid_old);
 	if (ac_old)
 		call_rcu(&ac_old->rcu, auditd_conn_free);
 
@@ -623,6 +618,7 @@ static void auditd_reset(const struct auditd_connection *ac)
 	unsigned long flags;
 	struct sk_buff *skb;
 	struct auditd_connection *ac_old;
+	struct pid *auditd_pid_old;
 
 	/* if it isn't already broken, break the connection */
 	spin_lock_irqsave(&auditd_conn_lock, flags);
@@ -634,9 +630,11 @@ static void auditd_reset(const struct auditd_connection *ac)
 		return;
 	}
 	rcu_assign_pointer(auditd_conn, NULL);
+	auditd_pid_old = auditd_pid;
 	WRITE_ONCE(auditd_pid, NULL);
 	spin_unlock_irqrestore(&auditd_conn_lock, flags);
 
+	put_pid(auditd_pid_old);
 	if (ac_old)
 		call_rcu(&ac_old->rcu, auditd_conn_free);
 
-- 
2.39.2

Re: [PATCH v2 1/2] audit: add global auditd_pid to make auditd_test_task() faster

[Paul Moore]

On Tue, Apr 18, 2023 at 7:10 AM Eiichi Tsukata
<eiichi.tsukata@nutanix.com> wrote:
>
> auditd_test_task() is a hot path of system call auditing. This patch
> introduces a global auditd_pid pid struct which can be used for faster
> check of registered audit daemon.
>
> Benchmarks
> ==========
>
> Run the following micro benchmarks:
>
>   (1) dd:
>     dd if=/dev/zero of=/dev/null bs=1 count=5M
>
>   (2) UnixBench syscall:
>     ./Run syscall -i 10 -c 1
>
> With rule:
>
>   -a never,exit -F arch=b64 -S uname
>
> Results:
>
>   (1) dd
>     Base line    : 2.572 sec
>     /w this patch: 2.418 sec (6.3% faster)
>
>   (2) UnixBench syscall Index Score
>     Base line    : 860
>     /w this patch: 953 (10.8% faster)
>
> Signed-off-by: Eiichi Tsukata <eiichi.tsukata@nutanix.com>
> ---
>  kernel/audit.c | 39 +++++++++++----------------------------
>  kernel/audit.h |  4 +++-
>  2 files changed, 14 insertions(+), 29 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 9bc0b0301198..9426980368e4 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -71,6 +71,7 @@ static int    audit_initialized = AUDIT_UNINITIALIZED;
>
>  u32            audit_enabled = AUDIT_OFF;
>  bool           audit_ever_enabled = !!AUDIT_OFF;
> +struct pid      *auditd_pid;

As discussed previously, I want to keep the auditd tracking PID in the
auditd_connection struct.

-- 
paul-moore.com