kernel/audit.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-)
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.6On 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
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
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
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
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
© 2016 - 2025 Red Hat, Inc.