Messages in this thread | | | Date | Wed, 18 Oct 2023 15:11:28 +0300 | Subject | Re: [PATCH v2] audit: Send netlink ACK before setting connection in auditd_set | From | 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
| |