| Message ID | 20231018092351.195715-1-chris.riches@nutanix.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v2] audit: Send netlink ACK before setting connection in auditd_set | expand |
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
Hi Paul, Is there any update on the review of the v2 patch? Thanks, Chris
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.
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
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.
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);