[RFC PATCH] audit: Send netlink ACK before setting connection in auditd_set

[RFC PATCH] audit: Send netlink ACK before setting connection in auditd_set

[Chris Riches]

When auditd_set sets the auditd_conn pointer, audit messages can
immediately be put on the socket by other kernel threads. If the backlog
is large or the rate is high, this can immediately fill the socket
buffer. If the audit daemon requested an ACK for this operation, a full
socket buffer causes the ACK to get dropped, also setting ENOBUFS on the
socket.

To avoid this race and ensure ACKs get through, fast-track the ACK in
this specific case to ensure it is sent before auditd_conn is set.

Signed-off-by: Chris Riches <chris.riches@nutanix.com>

---

This mail is more of an RFC than a patch, though the included patch is a
useful illustation and might even be suitable for inclusion.

We are experiencing strange failures where the audit daemon fails to
start, hitting an ENOBUFS error on its audit_set_pid() call. This can be
reproduced by repeatedly restarting the audit daemon (or just doing a
tight loop of audit_open(), audit_set_pid(pid), audit_set_pid(0),
audit_close()) while the system is under heavy audit load. This also
seems to be dependent on the number of CPUs - we can reproduce this with
2 CPUs but not with 48.

Tracing showed a race between the kernel setting auditd_conn and sending
the ACK, as described in the commit message. The included patch attempts
to fix this by ensuring the ACK is sent before any audit messages can be
put on the socket. This seems to fix the problem for auditd starting,
but strangely I still hit it when running my minimal repro (the tight
loop mentioned above), albeit less frequently. I'm not sure if the patch
or the repro is at fault.

We may also want to look at this from the userspace side in the audit
daemon itself (or more specifically, libaudit). The ACK handling is a
little odd - check_ack() will happily return success without seeing an
ACK if a non-ACK message is top of the socket queue, but will fail if no
message arrives within the timeout. It also of course fails if ENOBUFS
is set on the socket, but this failure only seems to matter when doing
audit_set_pid() - similar failures during main-loop message processing
are logged but otherwise ignored, as far as I can tell.

I'm not sure I quite understand the intentions of the code, but it seems
odd to let ENOBUFS be a fatal error here, given that it likely means the
socket buffer got flooded with audit messages, and thus audit_set_pid()
succeeded. Perhaps we should just ignore ENOBUFS (instead of or as well
as patching the kernel).

Finally, there is another oddity in audit_set_pid() that is tangential
to this discussion but worth highlighting: if the wmode parameter is
WAIT_YES (which it is for the audit daemon), then there is some
additional ACK-handling which waits for 100 milliseconds and eats the
top message of the socket queue if one arrives, without inspecting it.
This seems completely wrong as the ACK will have already been consumed
by check_ack() if there was one, and so the best this code can do is
nothing, and at worst it will swallow a genuine audit message without
ever recording it. If you agree with my assessment of this code, I'm
happy to submit a separate patch to fix this.

Thanks,
Chris

 kernel/audit.c | 30 ++++++++++++++++++++++++------
 kernel/audit.h |  5 +++++
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 9bc0b0301198..c48c778b6f89 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -488,12 +488,16 @@ static void auditd_conn_free(struct rcu_head *rcu)
  * @pid: auditd PID
  * @portid: auditd netlink portid
  * @net: auditd network namespace pointer
+ * @ack_status: if AUDIT_ACK_ALWAYS, send ACK before setting connection and
+ *              set to AUDIT_ACK_DONE
+ * @skb: socket buffer for sending ACK
  *
  * Description:
  * This function will obtain and drop network namespace references as
  * necessary.  Returns zero on success, negative values on failure.
  */
-static int auditd_set(struct pid *pid, u32 portid, struct net *net)
+static int auditd_set(struct pid *pid, u32 portid, struct net *net,
+                      int *ack_status, struct sk_buff *skb)
 {
 	unsigned long flags;
 	struct auditd_connection *ac_old, *ac_new;
@@ -508,6 +512,13 @@ static int auditd_set(struct pid *pid, u32 portid, struct net *net)
 	ac_new->portid = portid;
 	ac_new->net = get_net(net);
 
+	if (*ack_status == AUDIT_ACK_ALWAYS) {
+		// Send ACK before we set auditd_conn. Otherwise, the socket buffer may
+		// get filled with backlogged audit messages causing the ACK to be dropped.
+		netlink_ack(skb, nlmsg_hdr(skb), 0, NULL);
+		*ack_status = AUDIT_ACK_DONE;
+	}
+
 	spin_lock_irqsave(&auditd_conn_lock, flags);
 	ac_old = rcu_dereference_protected(auditd_conn,
 					   lockdep_is_held(&auditd_conn_lock));
@@ -1201,7 +1212,7 @@ static int audit_replace(struct pid *pid)
 	return auditd_send_unicast_skb(skb);
 }
 
-static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
+static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh, int *ack_status)
 {
 	u32			seq;
 	void			*data;
@@ -1294,7 +1305,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 				/* register a new auditd connection */
 				err = auditd_set(req_pid,
 						 NETLINK_CB(skb).portid,
-						 sock_net(NETLINK_CB(skb).sk));
+						 sock_net(NETLINK_CB(skb).sk),
+						 ack_status,
+						 skb);
 				if (audit_enabled != AUDIT_OFF)
 					audit_log_config_change("audit_pid",
 								new_pid,
@@ -1548,15 +1561,20 @@ static void audit_receive(struct sk_buff  *skb)
 	 */
 	int len;
 	int err;
+	int ack_status = AUDIT_ACK_ON_ERR;
 
 	nlh = nlmsg_hdr(skb);
 	len = skb->len;
 
 	audit_ctl_lock();
 	while (nlmsg_ok(nlh, len)) {
-		err = audit_receive_msg(skb, nlh);
-		/* if err or if this message says it wants a response */
-		if (err || (nlh->nlmsg_flags & NLM_F_ACK))
+		if (nlh->nlmsg_flags & NLM_F_ACK)
+			ack_status = AUDIT_ACK_ALWAYS;
+
+		err = audit_receive_msg(skb, nlh, &ack_status);
+
+		/* send ACK if err or the message asked for one (and not already sent) */
+		if (ack_status == AUDIT_ACK_ALWAYS || (ack_status == AUDIT_ACK_ON_ERR && err))
 			netlink_ack(skb, nlh, err, NULL);
 
 		nlh = nlmsg_next(nlh, &len);
diff --git a/kernel/audit.h b/kernel/audit.h
index 94738bce40b2..bc692f60567a 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -344,4 +344,9 @@ extern int audit_filter(int msgtype, unsigned int listtype);
 extern void audit_ctl_lock(void);
 extern void audit_ctl_unlock(void);
 
+/* Control netlink ACK behaviour in audit_receive. */
+#define AUDIT_ACK_ON_ERR 1
+#define AUDIT_ACK_ALWAYS 2
+#define AUDIT_ACK_DONE 3
+
 #endif
-- 
2.36.6

Re: [RFC PATCH] audit: Send netlink ACK before setting connection in auditd_set

[Paul Moore]

On Fri, Sep 22, 2023 at 11:28 AM Chris Riches <chris.riches@nutanix.com> wrote:
>
> When auditd_set sets the auditd_conn pointer, audit messages can
> immediately be put on the socket by other kernel threads. If the backlog
> is large or the rate is high, this can immediately fill the socket
> buffer. If the audit daemon requested an ACK for this operation, a full
> socket buffer causes the ACK to get dropped, also setting ENOBUFS on the
> socket.
>
> To avoid this race and ensure ACKs get through, fast-track the ACK in
> this specific case to ensure it is sent before auditd_conn is set.
>
> Signed-off-by: Chris Riches <chris.riches@nutanix.com>
>
> ---
>
> This mail is more of an RFC than a patch, though the included patch is a
> useful illustation and might even be suitable for inclusion ...

Hi Chris,

Thanks for the patch and the background information, that's always
helpful.  Unfortunately I have limited network access at the moment,
but I'll put this on my list to look at next week.

-- 
paul-moore.com

Re: [RFC PATCH] audit: Send netlink ACK before setting connection in auditd_set

[Paul Moore]

On Fri, Sep 22, 2023 at 11:28 AM Chris Riches <chris.riches@nutanix.com> wrote:
>
> When auditd_set sets the auditd_conn pointer, audit messages can
> immediately be put on the socket by other kernel threads. If the backlog
> is large or the rate is high, this can immediately fill the socket
> buffer. If the audit daemon requested an ACK for this operation, a full
> socket buffer causes the ACK to get dropped, also setting ENOBUFS on the
> socket.
>
> To avoid this race and ensure ACKs get through, fast-track the ACK in
> this specific case to ensure it is sent before auditd_conn is set.
>
> Signed-off-by: Chris Riches <chris.riches@nutanix.com>

Thanks for looking into this and submitting a patch Chris!

I think the basic approach is good, but I think we can simply things
slightly by using a resettable boolean as opposed to an integer flag
for the ACK.  I've pasted in a quick, untested patch (below) to better
demonstrate the idea, let me know what you think.  Further thoughts
below ...

> This mail is more of an RFC than a patch, though the included patch is a
> useful illustation and might even be suitable for inclusion.

Regardless of any other issues, I think you brought up a good point
about there being socket buffer contention when the queues are
full(ish) and an audit daemon connects to the kernel and while I'm not
sure that this patch will fully resolve that issue, I do think it
would be good to have (I'm doubtful if it can be fully resolved
without some really awful hacks).

> We are experiencing strange failures where the audit daemon fails to
> start, hitting an ENOBUFS error on its audit_set_pid() call. This can be
> reproduced by repeatedly restarting the audit daemon (or just doing a
> tight loop of audit_open(), audit_set_pid(pid), audit_set_pid(0),
> audit_close()) while the system is under heavy audit load. This also
> seems to be dependent on the number of CPUs - we can reproduce this with
> 2 CPUs but not with 48.

The ENOBUFS errors are coming from the netlink layer and are likely a
sign of extreme load.  I'm not very familiar with the audit userspace
code, but it might be helpful to see if you can increase the socket
buffer size for the audit daemon.

> Tracing showed a race between the kernel setting auditd_conn and sending
> the ACK, as described in the commit message. The included patch attempts
> to fix this by ensuring the ACK is sent before any audit messages can be
> put on the socket. This seems to fix the problem for auditd starting,
> but strangely I still hit it when running my minimal repro (the tight
> loop mentioned above), albeit less frequently. I'm not sure if the patch
> or the repro is at fault.

Sending a zero PID value, e.g. audit_set_pid(0), is going to cause the
kernel to reset/drop the connection with the audit daemon; it's
essentially the same as an orderly shutdown of the audit daemon.
However, if you do it in a tight loop alternating between
audit_set_pid(X) and audit_set_pid(0) you are basically thrashing the
audit daemon connection state in the kernel which is in turn likely
causing odd things to happen both with the kernel's audit queues and
the auditd socket buffer.

While the userspace reproducer is valid, it isn't something I would
expect in normal use as simply starting and stopping the audit daemon
adds a significant delay between disconnecting and reconnecting.

I'm also not surprised to hear that as you increase the number of CPUs
the problem goes away.  With more CPUs the system is able to schedule
more threads simultaneously to maintain the kernel's audit queues and
execute the audit daemon to drain both the netlink socket buffer and
audit queues.

> We may also want to look at this from the userspace side in the audit
> daemon itself (or more specifically, libaudit). The ACK handling is a
> little odd - check_ack() will happily return success without seeing an
> ACK if a non-ACK message is top of the socket queue, but will fail if no
> message arrives within the timeout. It also of course fails if ENOBUFS
> is set on the socket, but this failure only seems to matter when doing
> audit_set_pid() - similar failures during main-loop message processing
> are logged but otherwise ignored, as far as I can tell.

I suggest bringing this up with the audit userspace developer if you
haven't already.  While we can, and should, improve things on the
kernel side (e.g. the patch we are discussing), it also sounds like
the userspace side has room for improvement as well.

Here is the patch I mentioned above:

diff --git a/kernel/audit.c b/kernel/audit.c
index 16205dd29843..49f18003c1ac 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -487,15 +487,19 @@ static void auditd_conn_free(struct rcu_head *rcu)
 * @pid: auditd PID
 * @portid: auditd netlink portid
 * @net: auditd network namespace pointer
+ * @skb: the netlink command from the audit daemon
+ * @ack: netlink ack flag, cleared if ack'd here
 *
 * Description:
 * This function will obtain and drop network namespace references as
 * necessary.  Returns zero on success, negative values on failure.
 */
-static int auditd_set(struct pid *pid, u32 portid, struct net *net)
+static int auditd_set(struct pid *pid, u32 portid,
+                     struct net *net, struct sk_buff *skb, bool *ack)
{
       unsigned long flags;
       struct auditd_connection *ac_old, *ac_new;
+       struct nlmsghdr *nlh;

       if (!pid || !net)
               return -EINVAL;
@@ -507,6 +511,13 @@ static int auditd_set(struct pid *pid, u32 portid, struct n
et *net)
       ac_new->portid = portid;
       ac_new->net = get_net(net);

+       /* send the ack now to avoid a race with the queue backlog */
+       if (*ack) {
+               nlh = nlmsg_hdr(skb);
+               netlink_ack(skb, nlh, 0, NULL);
+               *ack = false;
+       }
+
       spin_lock_irqsave(&auditd_conn_lock, flags);
       ac_old = rcu_dereference_protected(auditd_conn,
                                          lockdep_is_held(&auditd_conn_lock));
@@ -1200,7 +1211,8 @@ static int audit_replace(struct pid *pid)
       return auditd_send_unicast_skb(skb);
}

-static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
+static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
+                            bool *ack)
{
       u32                     seq;
       void                    *data;
@@ -1293,7 +1305,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct n
lmsghdr *nlh)
                               /* register a new auditd connection */
                               err = auditd_set(req_pid,
                                                NETLINK_CB(skb).portid,
-                                                sock_net(NETLINK_CB(skb).sk));
+                                                sock_net(NETLINK_CB(skb).sk),
+                                                skb, ack);
                               if (audit_enabled != AUDIT_OFF)
                                       audit_log_config_change("audit_pid",
                                                               new_pid,
@@ -1538,9 +1551,10 @@ static int audit_receive_msg(struct sk_buff *skb, struct
nlmsghdr *nlh)
 * Parse the provided skb and deal with any messages that may be present,
 * malformed skbs are discarded.
 */
-static void audit_receive(struct sk_buff  *skb)
+static void audit_receive(struct sk_buff *skb)
{
       struct nlmsghdr *nlh;
+       bool ack;
       /*
        * len MUST be signed for nlmsg_next to be able to dec it below 0
        * if the nlmsg_len was not aligned
@@ -1553,9 +1567,13 @@ static void audit_receive(struct sk_buff  *skb)

       audit_ctl_lock();
       while (nlmsg_ok(nlh, len)) {
-               err = audit_receive_msg(skb, nlh);
-               /* if err or if this message says it wants a response */
-               if (err || (nlh->nlmsg_flags & NLM_F_ACK))
+               ack = nlh->nlmsg_flags & NLM_F_ACK;
+               err = audit_receive_msg(skb, nlh, &ack);
+
+               /* if audit_receive_msg() clears @ack then we never want to
+                * send an ack here, otherwise send an ack on error or if
+                * requested */
+               if (ack && (err || (nlh->nlmsg_flags & NLM_F_ACK)))
                       netlink_ack(skb, nlh, err, NULL);

               nlh = nlmsg_next(nlh, &len);

-- 
paul-moore.com

Re: [RFC PATCH] audit: Send netlink ACK before setting connection in auditd_set

[Chris Riches]

Hi Paul, thanks for the reply.


 > I think the basic approach is good, but I think we can simply things
 > slightly by using a resettable boolean as opposed to an integer flag
 > for the ACK.  I've pasted in a quick, untested patch (below) to better
 > demonstrate the idea, let me know what you think.

The simplified patch doesn't look quite right. I had some trouble
getting it to apply (seems tabs were changed into spaces, even when I
downloaded the raw email). While typing it out manually, I noticed that
the condition for sending the ACK isn't correct - if NLM_F_ACK is 0 to
begin with, then ack will be false to begin with, and so no ACK will be
sent even if there is an error. Handling this case is why I used the
three-state system to begin with. I think we could still use a boolean
with a condition of just (err || ack), with the caveat that we wouldn't
easily be able to send an early ACK on an error (not that the current
patch needs this - just thinking of reusability).


 > Regardless of any other issues, I think you brought up a good point
 > about there being socket buffer contention when the queues are
 > full(ish) and an audit daemon connects to the kernel and while I'm not
 > sure that this patch will fully resolve that issue, I do think it
 > would be good to have (I'm doubtful if it can be fully resolved
 > without some really awful hacks).

That roughly lines up with what I was seeing - could you elaborate any
further on what "awful hacks" would be needed to fully resolve this?
I'm intrigued as to where else the contention could be coming from that
wouldn't be covered by this patch.


 > The ENOBUFS errors are coming from the netlink layer and are likely a
 > sign of extreme load.  I'm not very familiar with the audit userspace
 > code, but it might be helpful to see if you can increase the socket
 > buffer size for the audit daemon.

I believe we tried increasing the buffer size in the toy repro and it
didn't make much difference - perhaps doing it in the real audit daemon
might help.


 > I'm also not surprised to hear that as you increase the number of CPUs
 > the problem goes away.  With more CPUs the system is able to schedule
 > more threads simultaneously to maintain the kernel's audit queues and
 > execute the audit daemon to drain both the netlink socket buffer and
 > audit queues.

My intuition told me the opposite - that more parallel activity would
increase the chance of the socket buffer being crammed full before the
one thread could return to the audit daemon and allow it to start
processing events. Does the size/number of audit queues perhaps scale
with the number of CPUs?


 > I suggest bringing this up with the audit userspace developer if you
 > haven't already.  While we can, and should, improve things on the
 > kernel side (e.g. the patch we are discussing), it also sounds like
 > the userspace side has room for improvement as well.

That sounds like a good idea - can you point me to who that is? I was
under the impression that this was the mailing list for both the kernel
and userspace sides.


Thanks,
Chris

Re: [RFC PATCH] audit: Send netlink ACK before setting connection in auditd_set

[Paul Moore]

On Mon, Oct 16, 2023 at 1:12 PM Chris Riches <chris.riches@nutanix.com> wrote:
>

Thanks for trimming the email in your reply, however, it is helpful to
preserve those "On Mon, Oct ..." headers for those emails which you
include in your reply, it helps keep things straight when reading the
email.  Not a big deal, just something to keep in mind for next time.

>  > I think the basic approach is good, but I think we can simply things
>  > slightly by using a resettable boolean as opposed to an integer flag
>  > for the ACK.  I've pasted in a quick, untested patch (below) to better
>  > demonstrate the idea, let me know what you think.
>
> The simplified patch doesn't look quite right. I had some trouble
> getting it to apply (seems tabs were changed into spaces, even when I
> downloaded the raw email).

I should have been more clear, that's what just a quick hack that I
cut-n-pasted into the email body, whitespace damage was a given.
Typically if I include a patch with the qualification that it is
untested, you can expect problems :) but I'll try to make the pitfalls
more explicit in the future.

I usually include these hacky patches simply as a way to help clear up
any misunderstandings that might happen when trying to explain an
approach using English descriptions.  Much in the same way we say that
a picture is worth a thousand words, I believe a patch, even a
relatively crude one, is worth at least as many words as a picture :)

> While typing it out manually, I noticed that
> the condition for sending the ACK isn't correct - if NLM_F_ACK is 0 to
> begin with, then ack will be false to begin with, and so no ACK will be
> sent even if there is an error.

Good point.  I'll just casually remind you that I did say "untested" ;)

I believe the following should work as intended (untested, cut-n-paste, etc.):

@@ -1538,9 +1551,10 @@ static int audit_receive_msg(struct sk_buff *skb, struct
nlmsghdr *nlh)
 * Parse the provided skb and deal with any messages that may be present,
 * malformed skbs are discarded.
 */
-static void audit_receive(struct sk_buff  *skb)
+static void audit_receive(struct sk_buff *skb)
{
       struct nlmsghdr *nlh;
+       bool ack;
       /*
        * len MUST be signed for nlmsg_next to be able to dec it below 0
        * if the nlmsg_len was not aligned
@@ -1553,9 +1567,13 @@ static void audit_receive(struct sk_buff  *skb)

       audit_ctl_lock();
       while (nlmsg_ok(nlh, len)) {
-               err = audit_receive_msg(skb, nlh);
-               /* if err or if this message says it wants a response */
-               if (err || (nlh->nlmsg_flags & NLM_F_ACK))
+               ack = true;
+               err = audit_receive_msg(skb, nlh, &ack);
+
+               /* if audit_receive_msg() clears @ack then we never want to
+                * send an ack here, otherwise send an ack on error or if
+                * requested */
+               if (ack && (err || (nlh->nlmsg_flags & NLM_F_ACK)))
                       netlink_ack(skb, nlh, err, NULL);

> Handling this case is why I used the
> three-state system to begin with. I think we could still use a boolean
> with a condition of just (err || ack), with the caveat that we wouldn't
> easily be able to send an early ACK on an error (not that the current
> patch needs this - just thinking of reusability).

While there may be three states (never ACK, ACK on error, always ACK),
the audit_receive_msg() function only needs to report back one of two
states: never ACK or ACK if desired.

>  > Regardless of any other issues, I think you brought up a good point
>  > about there being socket buffer contention when the queues are
>  > full(ish) and an audit daemon connects to the kernel and while I'm not
>  > sure that this patch will fully resolve that issue, I do think it
>  > would be good to have (I'm doubtful if it can be fully resolved
>  > without some really awful hacks).
>
> That roughly lines up with what I was seeing - could you elaborate any
> further on what "awful hacks" would be needed to fully resolve this?

I'm not sure I can recall everything from when I was thinking about
this previously (that was about a week ago), but my quick thoughts
right now are that you would need a lot more information and/or
handshakes between the kernel and the daemon.

Unfortunately, both the current audit design and implementation is
seriously flawed in a number of areas.  One of these areas is the fact
that data and control messages are sent using the same data flow.

>  > The ENOBUFS errors are coming from the netlink layer and are likely a
>  > sign of extreme load.  I'm not very familiar with the audit userspace
>  > code, but it might be helpful to see if you can increase the socket
>  > buffer size for the audit daemon.
>
> I believe we tried increasing the buffer size in the toy repro and it
> didn't make much difference - perhaps doing it in the real audit daemon
> might help.

The reproducer is an artificial worst case since you are disconnecting
and reconnecting without a process shutdown and startup in between.
The reproducer just hammers the connection status as fast as the CPU
can generate netlink messages; I'm not certain there is a
netlink/socket buffer size that would fully resolve this for the
reproducer.

>  > I'm also not surprised to hear that as you increase the number of CPUs
>  > the problem goes away.  With more CPUs the system is able to schedule
>  > more threads simultaneously to maintain the kernel's audit queues and
>  > execute the audit daemon to drain both the netlink socket buffer and
>  > audit queues.
>
> My intuition told me the opposite - that more parallel activity would
> increase the chance of the socket buffer being crammed full before the
> one thread could return to the audit daemon and allow it to start
> processing events.

The audit subsystem can handle a full buffer, that has been stressed a
lot over the years, and while it may slow the system down it will
continue to operate.  As a fun exercise, configure the audit subsystem
to audit *every* syscall and then try to shut the system down.  It
will complete, but it's ugly and you'll see the audit subsystem
complain a *lot* about dropped records due to queue/backlog overflows.

The issue isn't so much about the queues overflowing inside the
kernel, it's about being able to schedule the audit daemon and/or
kernel thread to service the flood of connection
disconnects/reconnects coming from the reproducer.

> Does the size/number of audit queues perhaps scale
> with the number of CPUs?

The number of queues is static, and their size is determined at
runtime by the configuration.  Making per-CPU queues would be a bit of
a mess due to our desire to ensure an accurate ordering of events
relative to a wall clock.

>  > I suggest bringing this up with the audit userspace developer if you
>  > haven't already.  While we can, and should, improve things on the
>  > kernel side (e.g. the patch we are discussing), it also sounds like
>  > the userspace side has room for improvement as well.
>
> That sounds like a good idea - can you point me to who that is? I was
> under the impression that this was the mailing list for both the kernel
> and userspace sides.

That used to be the case, but unfortunately the audit userspace
developer who manages the original audit mailing list was unwilling to
make the list configuration changes we needed to play nice with
existing Linux kernel mailing list conventions so we needed to move
the kernel development discussions to a separate list.

The old audit mailing list, where the userspace development is still
discussed, can be found here:

* http://www.redhat.com/mailman/listinfo/linux-audit

-- 
paul-moore.com

Re: [RFC PATCH] audit: Send netlink ACK before setting connection in auditd_set

[Chris Riches]

On 16/10/2023 21:16, Paul Moore wrote:

 > Thanks for trimming the email in your reply, however, it is helpful to
 > preserve those "On Mon, Oct ..." headers for those emails which you
 > include in your reply, it helps keep things straight when reading the
 > email.  Not a big deal, just something to keep in mind for next time.

Thanks for the pointer - I'm new to these mailing lists so appreciate
the advice.


 > I should have been more clear, that's what just a quick hack that I
 > cut-n-pasted into the email body, whitespace damage was a given.
 > Typically if I include a patch with the qualification that it is
 > untested, you can expect problems :) but I'll try to make the pitfalls
 > more explicit in the future.

Gotcha.


 >> While typing it out manually, I noticed that
 >> the condition for sending the ACK isn't correct - if NLM_F_ACK is 0 to
 >> begin with, then ack will be false to begin with, and so no ACK will be
 >> sent even if there is an error.
 >
 > Good point.  I'll just casually remind you that I did say "untested" ;)
 >
 > I believe the following should work as intended (untested, 
cut-n-paste, etc.):
 > .....

I think ack must be set to NLM_F_ACK initially - otherwise auditd_set
will always send the fast-tracked ACK even if the caller did not
request one. The following is a concrete version of what I roughly
suggested in the last email - is there a specific problem you see with
the (ack || err) condition?

@@ -1538,9 +1551,10 @@ static int audit_receive_msg(struct sk_buff *skb, 
struct
nlmsghdr *nlh)
  * Parse the provided skb and deal with any messages that may be present,
  * malformed skbs are discarded.
  */
-static void audit_receive(struct sk_buff  *skb)
+static void audit_receive(struct sk_buff *skb)
  {
      struct nlmsghdr *nlh;
+    bool ack;
      /*
       * len MUST be signed for nlmsg_next to be able to dec it below 0
       * if the nlmsg_len was not aligned
@@ -1553,9 +1567,13 @@ static void audit_receive(struct sk_buff *skb)

      audit_ctl_lock();
      while (nlmsg_ok(nlh, len)) {
-        err = audit_receive_msg(skb, nlh);
-        /* if err or if this message says it wants a response */
-        if (err || (nlh->nlmsg_flags & NLM_F_ACK))
+        ack = nlh->nlmsg_flags & NLM_F_ACK;
+        err = audit_receive_msg(skb, nlh, &ack);
+
+        /* Send an ack if @ack is still true after audit_receive_msg
+         * potentially cleared it, or if there was an error. */
+        if (ack || err)
              netlink_ack(skb, nlh, err, NULL);


 > I'm not sure I can recall everything from when I was thinking about
 > this previously (that was about a week ago), but my quick thoughts
 > right now are that you would need a lot more information and/or
 > handshakes between the kernel and the daemon.
 >
 > Unfortunately, both the current audit design and implementation is
 > seriously flawed in a number of areas.  One of these areas is the fact
 > that data and control messages are sent using the same data flow.

Makes sense. The question of why there isn't a separate control socket
was one of the first we asked while looking into this.


 > The issue isn't so much about the queues overflowing inside the
 > kernel, it's about being able to schedule the audit daemon and/or
 > kernel thread to service the flood of connection
 > disconnects/reconnects coming from the reproducer.

Right, makes sense.


 > The old audit mailing list, where the userspace development is still
 > discussed, can be found here:
 > ...

Thanks. I'll post there too.


- Chris

Re: [RFC PATCH] audit: Send netlink ACK before setting connection in auditd_set

[Paul Moore]

On Tue, Oct 17, 2023 at 9:49 AM Chris Riches <chris.riches@nutanix.com> wrote:
> On 16/10/2023 21:16, Paul Moore wrote:
>  >> While typing it out manually, I noticed that
>  >> the condition for sending the ACK isn't correct - if NLM_F_ACK is 0 to
>  >> begin with, then ack will be false to begin with, and so no ACK will be
>  >> sent even if there is an error.
>  >
>  > Good point.  I'll just casually remind you that I did say "untested" ;)
>  >
>  > I believe the following should work as intended (untested,
> cut-n-paste, etc.):
>  > .....
>
> I think ack must be set to NLM_F_ACK initially - otherwise auditd_set
> will always send the fast-tracked ACK even if the caller did not
> request one.

Another good point.  I think I'm working on too many different things
and forgetting the details between our emails :)  Sorry about that.

> The following is a concrete version of what I roughly
> suggested in the last email - is there a specific problem you see with
> the (ack || err) condition?

I'm happier with the bool over the integer type for the @ack variable.
I'd suggest updating the patch and posting it again so we can review
it in full, but we weren't very far off last time so I think it should
be close, if not acceptable on the next revision.

>  > I'm not sure I can recall everything from when I was thinking about
>  > this previously (that was about a week ago), but my quick thoughts
>  > right now are that you would need a lot more information and/or
>  > handshakes between the kernel and the daemon.
>  >
>  > Unfortunately, both the current audit design and implementation is
>  > seriously flawed in a number of areas.  One of these areas is the fact
>  > that data and control messages are sent using the same data flow.
>
> Makes sense. The question of why there isn't a separate control socket
> was one of the first we asked while looking into this.

Yeah, there is a lot I would have done differently, but that's the way
it goes sometimes.  I have plans to find some of the larger problems,
I just need to find some time to work on it.

-- 
paul-moore.com

[PATCH v2] audit: Send netlink ACK before setting connection in auditd_set

[Chris Riches]

When auditd_set sets the auditd_conn pointer, audit messages can
immediately be put on the socket by other kernel threads. If the backlog
is large or the rate is high, this can immediately fill the socket
buffer. If the audit daemon requested an ACK for this operation, a full
socket buffer causes the ACK to get dropped, also setting ENOBUFS on the
socket.

To avoid this race and ensure ACKs get through, fast-track the ACK in
this specific case to ensure it is sent before auditd_conn is set.

Signed-off-by: Chris Riches <chris.riches@nutanix.com>

---

> I'm happier with the bool over the integer type for the @ack variable.
> I'd suggest updating the patch and posting it again so we can review
> it in full, but we weren't very far off last time so I think it should
> be close, if not acceptable on the next revision.

Here's the latest iteration of the suggested patch. I've done it via git
send-email so it should apply cleanly.



 kernel/audit.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 9bc0b0301198..20c6981490ad 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -488,15 +488,19 @@ static void auditd_conn_free(struct rcu_head *rcu)
  * @pid: auditd PID
  * @portid: auditd netlink portid
  * @net: auditd network namespace pointer
+ * @skb: the netlink command from the audit daemon
+ * @ack: netlink ack flag, cleared if ack'd here
  *
  * Description:
  * This function will obtain and drop network namespace references as
  * necessary.  Returns zero on success, negative values on failure.
  */
-static int auditd_set(struct pid *pid, u32 portid, struct net *net)
+static int auditd_set(struct pid *pid, u32 portid, struct net *net,
+                      struct sk_buff *skb, bool *ack)
 {
 	unsigned long flags;
 	struct auditd_connection *ac_old, *ac_new;
+	struct nlmsghdr *nlh;
 
 	if (!pid || !net)
 		return -EINVAL;
@@ -508,6 +512,13 @@ static int auditd_set(struct pid *pid, u32 portid, struct net *net)
 	ac_new->portid = portid;
 	ac_new->net = get_net(net);
 
+	/* send the ack now to avoid a race with the queue backlog */
+	if (*ack) {
+		nlh = nlmsg_hdr(skb);
+		netlink_ack(skb, nlh, 0, NULL);
+		*ack = false;
+	}
+
 	spin_lock_irqsave(&auditd_conn_lock, flags);
 	ac_old = rcu_dereference_protected(auditd_conn,
 					   lockdep_is_held(&auditd_conn_lock));
@@ -1201,7 +1212,8 @@ static int audit_replace(struct pid *pid)
 	return auditd_send_unicast_skb(skb);
 }
 
-static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
+static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
+			     bool *ack)
 {
 	u32			seq;
 	void			*data;
@@ -1294,7 +1306,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 				/* register a new auditd connection */
 				err = auditd_set(req_pid,
 						 NETLINK_CB(skb).portid,
-						 sock_net(NETLINK_CB(skb).sk));
+						 sock_net(NETLINK_CB(skb).sk),
+						 skb, ack);
 				if (audit_enabled != AUDIT_OFF)
 					audit_log_config_change("audit_pid",
 								new_pid,
@@ -1539,9 +1552,10 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
  * Parse the provided skb and deal with any messages that may be present,
  * malformed skbs are discarded.
  */
-static void audit_receive(struct sk_buff  *skb)
+static void audit_receive(struct sk_buff *skb)
 {
 	struct nlmsghdr *nlh;
+	bool ack;
 	/*
 	 * len MUST be signed for nlmsg_next to be able to dec it below 0
 	 * if the nlmsg_len was not aligned
@@ -1554,9 +1568,12 @@ static void audit_receive(struct sk_buff  *skb)
 
 	audit_ctl_lock();
 	while (nlmsg_ok(nlh, len)) {
-		err = audit_receive_msg(skb, nlh);
-		/* if err or if this message says it wants a response */
-		if (err || (nlh->nlmsg_flags & NLM_F_ACK))
+		ack = nlh->nlmsg_flags & NLM_F_ACK;
+		err = audit_receive_msg(skb, nlh, &ack);
+
+		/* send an ack if the user asked for one and audit_receive_msg
+		 * didn't already do it, or if there was an error. */
+		if (ack || err)
 			netlink_ack(skb, nlh, err, NULL);
 
 		nlh = nlmsg_next(nlh, &len);
-- 
2.36.6

Re: [PATCH v2] audit: Send netlink ACK before setting connection in auditd_set

[Rinat Gadelshin]

Hello there!

On 18.10.2023 12:23, Chris Riches wrote:
> When auditd_set sets the auditd_conn pointer, audit messages can
> immediately be put on the socket by other kernel threads. If the backlog
> is large or the rate is high, this can immediately fill the socket
> buffer. If the audit daemon requested an ACK for this operation, a full
> socket buffer causes the ACK to get dropped, also setting ENOBUFS on the
> socket.
>
> To avoid this race and ensure ACKs get through, fast-track the ACK in
> this specific case to ensure it is sent before auditd_conn is set.
>
> Signed-off-by: Chris Riches <chris.riches@nutanix.com>
>
> ---
>
>> I'm happier with the bool over the integer type for the @ack variable.
>> I'd suggest updating the patch and posting it again so we can review
>> it in full, but we weren't very far off last time so I think it should
>> be close, if not acceptable on the next revision.
> Here's the latest iteration of the suggested patch. I've done it via git
> send-email so it should apply cleanly.
>
>
>
>   kernel/audit.c | 31 ++++++++++++++++++++++++-------
>   1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 9bc0b0301198..20c6981490ad 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -488,15 +488,19 @@ static void auditd_conn_free(struct rcu_head *rcu)
>    * @pid: auditd PID
>    * @portid: auditd netlink portid
>    * @net: auditd network namespace pointer
> + * @skb: the netlink command from the audit daemon
> + * @ack: netlink ack flag, cleared if ack'd here
>    *
>    * Description:
>    * This function will obtain and drop network namespace references as
>    * necessary.  Returns zero on success, negative values on failure.
>    */
> -static int auditd_set(struct pid *pid, u32 portid, struct net *net)
> +static int auditd_set(struct pid *pid, u32 portid, struct net *net,
> +                      struct sk_buff *skb, bool *ack)
>   {
>   	unsigned long flags;
>   	struct auditd_connection *ac_old, *ac_new;
> +	struct nlmsghdr *nlh;
>   
>   	if (!pid || !net)
>   		return -EINVAL;
> @@ -508,6 +512,13 @@ static int auditd_set(struct pid *pid, u32 portid, struct net *net)
>   	ac_new->portid = portid;
>   	ac_new->net = get_net(net);
>   
> +	/* send the ack now to avoid a race with the queue backlog */
> +	if (*ack) {
> +		nlh = nlmsg_hdr(skb);
> +		netlink_ack(skb, nlh, 0, NULL);
> +		*ack = false;
> +	}
> +
>   	spin_lock_irqsave(&auditd_conn_lock, flags);
>   	ac_old = rcu_dereference_protected(auditd_conn,
>   					   lockdep_is_held(&auditd_conn_lock));
> @@ -1201,7 +1212,8 @@ static int audit_replace(struct pid *pid)
>   	return auditd_send_unicast_skb(skb);
>   }
>   
> -static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> +static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
> +			     bool *ack)
>   {
>   	u32			seq;
>   	void			*data;
> @@ -1294,7 +1306,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>   				/* register a new auditd connection */
>   				err = auditd_set(req_pid,
>   						 NETLINK_CB(skb).portid,
> -						 sock_net(NETLINK_CB(skb).sk));
> +						 sock_net(NETLINK_CB(skb).sk),
> +						 skb, ack);
>   				if (audit_enabled != AUDIT_OFF)
>   					audit_log_config_change("audit_pid",
>   								new_pid,
> @@ -1539,9 +1552,10 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>    * Parse the provided skb and deal with any messages that may be present,
>    * malformed skbs are discarded.
>    */
> -static void audit_receive(struct sk_buff  *skb)
> +static void audit_receive(struct sk_buff *skb)
>   {
>   	struct nlmsghdr *nlh;
> +	bool ack;
Maybe we should initialize 'ack' as 'true' here?
>   	/*
>   	 * len MUST be signed for nlmsg_next to be able to dec it below 0
>   	 * if the nlmsg_len was not aligned
> @@ -1554,9 +1568,12 @@ static void audit_receive(struct sk_buff  *skb)
>   
>   	audit_ctl_lock();
>   	while (nlmsg_ok(nlh, len)) {
> -		err = audit_receive_msg(skb, nlh);
> -		/* if err or if this message says it wants a response */
> -		if (err || (nlh->nlmsg_flags & NLM_F_ACK))
> +		ack = nlh->nlmsg_flags & NLM_F_ACK;
> +		err = audit_receive_msg(skb, nlh, &ack);
> +
> +		/* send an ack if the user asked for one and audit_receive_msg
> +		 * didn't already do it, or if there was an error. */
> +		if (ack || err)
>   			netlink_ack(skb, nlh, err, NULL);
>   
>   		nlh = nlmsg_next(nlh, &len);


Best regards
Rinat

Re: [PATCH v2] audit: Send netlink ACK before setting connection in auditd_set

[Chris Riches]

On 18/10/2023 13:11, Rinat Gadelshin wrote:
>> -static void audit_receive(struct sk_buff *skb)
>> +static void audit_receive(struct sk_buff *skb)
>>   {
>>       struct nlmsghdr *nlh;
>> +    bool ack;
> Maybe we should initialize 'ack' as 'true' here?
That doesn't feel particularly useful to me. In fact, I think it's 
actually clearer
uninitialised as a never-used initialisation could look like an 
actually-used default.
We're guaranteed to initialise before use.

- Chris

Re: [PATCH v2] audit: Send netlink ACK before setting connection in auditd_set

[Rinat Gadelshin]

On 18.10.2023 15:49, Chris Riches wrote:
>
> On 18/10/2023 13:11, Rinat Gadelshin wrote:
>>> -static void audit_receive(struct sk_buff *skb)
>>> +static void audit_receive(struct sk_buff *skb)
>>>   {
>>>       struct nlmsghdr *nlh;
>>> +    bool ack;
>> Maybe we should initialize 'ack' as 'true' here?
> That doesn't feel particularly useful to me. In fact, I think it's 
> actually clearer
> uninitialised as a never-used initialisation could look like an 
> actually-used default.
> We're guaranteed to initialise before use.
>
> - Chris

Sorry, you are right.
I've missed the following line:

ack = nlh->nlmsg_flags & NLM_F_ACK;

- Rinat

Re: [PATCH v2] audit: Send netlink ACK before setting connection in auditd_set

[Chris Riches]

Hi Paul,

Is there any update on the review of the v2 patch?

Thanks,
Chris

Re: [PATCH v2] audit: Send netlink ACK before setting connection in auditd_set

[Paul Moore]

On Wed, Nov 1, 2023 at 5:59 AM Chris Riches <chris.riches@nutanix.com> wrote:
>
> Hi Paul,
>
> Is there any update on the review of the v2 patch?

Hi Chris,

I apologize for the delay, this is in my review queue, there is simply
a lot going on at the moment and I haven't been able to make as much
progress as I would like.

-- 
paul-moore.com

Re: [PATCH v2] audit: Send netlink ACK before setting connection in auditd_set

[Paul Moore]

On Oct 18, 2023 Paul Moore <paul@paul-moore.com> wrote:
> 
> When auditd_set sets the auditd_conn pointer, audit messages can
> immediately be put on the socket by other kernel threads. If the backlog
> is large or the rate is high, this can immediately fill the socket
> buffer. If the audit daemon requested an ACK for this operation, a full
> socket buffer causes the ACK to get dropped, also setting ENOBUFS on the
> socket.
> 
> To avoid this race and ensure ACKs get through, fast-track the ACK in
> this specific case to ensure it is sent before auditd_conn is set.
> 
> Signed-off-by: Chris Riches <chris.riches@nutanix.com>
> ---
> 
> > I'm happier with the bool over the integer type for the @ack variable.
> > I'd suggest updating the patch and posting it again so we can review
> > it in full, but we weren't very far off last time so I think it should
> > be close, if not acceptable on the next revision.
> 
> Here's the latest iteration of the suggested patch. I've done it via git
> send-email so it should apply cleanly.
> 
>  kernel/audit.c | 31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)

This looks good to me, thanks for your patience and persistence Chris.

I'm going to merge this into audit/dev-staging now and it will move to
audit/dev once v6.7-rc1 is released and the merge window closes.

--
paul-moore.com

Re: [PATCH v2] audit: Send netlink ACK before setting connection in auditd_set

[Paul Moore]

On Tue, Nov 7, 2023 at 6:31 PM Paul Moore <paul@paul-moore.com> wrote:
> On Oct 18, 2023 Paul Moore <paul@paul-moore.com> wrote:
> >
> > When auditd_set sets the auditd_conn pointer, audit messages can
> > immediately be put on the socket by other kernel threads. If the backlog
> > is large or the rate is high, this can immediately fill the socket
> > buffer. If the audit daemon requested an ACK for this operation, a full
> > socket buffer causes the ACK to get dropped, also setting ENOBUFS on the
> > socket.
> >
> > To avoid this race and ensure ACKs get through, fast-track the ACK in
> > this specific case to ensure it is sent before auditd_conn is set.
> >
> > Signed-off-by: Chris Riches <chris.riches@nutanix.com>
> > ---
> >
> > > I'm happier with the bool over the integer type for the @ack variable.
> > > I'd suggest updating the patch and posting it again so we can review
> > > it in full, but we weren't very far off last time so I think it should
> > > be close, if not acceptable on the next revision.
> >
> > Here's the latest iteration of the suggested patch. I've done it via git
> > send-email so it should apply cleanly.
> >
> >  kernel/audit.c | 31 ++++++++++++++++++++++++-------
> >  1 file changed, 24 insertions(+), 7 deletions(-)
>
> This looks good to me, thanks for your patience and persistence Chris.
>
> I'm going to merge this into audit/dev-staging now and it will move to
> audit/dev once v6.7-rc1 is released and the merge window closes.

I've now merged this into lsm/dev, thanks again Chris.

-- 
paul-moore.com