On 14 August 2026 02:01:53 BST, Bradley Morgan <[email protected]> wrote: >Right now the auditd breakage (nlmsg_len gets set to the payload >length instead of the full message length) is applied when the record >is queued, in __audit_log_end(). That is why >kauditd_send_multicast_skb() has to deep copy every record and then >undo the length on the copy, just so the multicast group still sees a >standard netlink message. > >So flip it: finalize the header with the standard full length at >queue time, and apply the auditd length at send time in >kauditd_send_queue(), right before the unicast. Records stay standard >netlink messages the whole time they sit in the queues, and the >multicast copy stops needing its own fixup. The copy itself stays, >because the rewrite still lands in the data region the listeners >already hold. > >auditd sees the same bytes as before: the fixup is computed from >skb->len and that does not change between queueing and sending, so >records that come back around through the retry and hold queues get >the same value again. Reply and rule list skbs are built with >nlmsg_put() and go out on their own paths, none of that is touched. > >This came out of reviewing Ricardo's "use copied skb length" patch, >where I suggested moving the fixup as the more interesting cleanup. > >Reviewed-by: Ricardo Robaina <[email protected]> >Tested-by: Ricardo Robaina <[email protected]> >Signed-off-by: Bradley Morgan <[email protected]> >Link: https://lore.kernel.org/r/[email protected] >--- > kernel/audit.c | 30 +++++++++++++----------------- > 1 file changed, 13 insertions(+), 17 deletions(-) > >diff --git a/kernel/audit.c b/kernel/audit.c >index 9412af9144bc..bcfed6e3678e 100644 >--- a/kernel/audit.c >+++ b/kernel/audit.c >@@ -802,6 +802,12 @@ static int kauditd_send_queue(struct sock *sk, u32 portid, > if (skb_hook) > (*skb_hook)(skb); > >+ /* >+ * auditd wants nlmsg_len to be the payload length, not the >+ * full length, so break it here at send time. >+ */ >+ nlmsg_hdr(skb)->nlmsg_len = skb->len - NLMSG_HDRLEN; >+ > /* can we send to anyone via unicast? */ > if (!sk) { > if (err_hook) >@@ -849,7 +855,6 @@ static void kauditd_send_multicast_skb(struct sk_buff *skb) > { > struct sk_buff *copy; > struct sock *sock = audit_get_sk(&init_net); >- struct nlmsghdr *nlh; > > /* NOTE: we are not taking an additional reference for init_net since > * we don't have to worry about it going away */ >@@ -858,20 +863,12 @@ static void kauditd_send_multicast_skb(struct sk_buff >*skb) > return; > > /* >- * The seemingly wasteful skb_copy() rather than bumping the refcount >- * using skb_get() is necessary because non-standard mods are made to >- * the skb by the original kaudit unicast socket send routine. The >- * existing auditd daemon assumes this breakage. Fixing this would >- * require co-ordinating a change in the established protocol between >- * the kaudit kernel subsystem and the auditd userspace code. There is >- * no reason for new multicast clients to continue with this >- * non-compliance. >+ * skb_copy() rather than skb_get(): kauditd_send_queue() breaks >+ * nlmsg_len for auditd, keep the listeners on a standard message. > */ > copy = skb_copy(skb, GFP_KERNEL); > if (!copy) > return; >- nlh = nlmsg_hdr(copy); >- nlh->nlmsg_len = skb->len; > > nlmsg_multicast(sock, copy, 0, AUDIT_NLGRP_READLOG, GFP_KERNEL); > } >@@ -2785,13 +2782,12 @@ int audit_signal_info(int sig, struct task_struct *t) > */ > static void __audit_log_end(struct sk_buff *skb) > { >- struct nlmsghdr *nlh; >- > if (audit_rate_check()) { >- /* setup the netlink header, see the comments in >- * kauditd_send_multicast_skb() for length quirks */ >- nlh = nlmsg_hdr(skb); >- nlh->nlmsg_len = skb->len - NLMSG_HDRLEN; >+ /* >+ * Records are built without keeping nlmsg_len up to date, >+ * finalize it here with the full message length. >+ */ >+ nlmsg_end(skb, nlmsg_hdr(skb)); > > /* queue the netlink packet */ > skb_queue_tail(&audit_queue, skb); > add akpm:, I seem to be getting ignored on these kind of patches, very annoyingly, any reason why in particular?
I'm getting slightly, could say angry, could say annoyed, either one. Thanks!
