Re: [PATCH] audit: move the nlmsg_len fixup from __audit_log_end() to send time

[Bradley Morgan]

On 20 August 2026 16:57:12 BST, Ricardo Robaina <rrobaina@xxxxxxxxxx>
wrote:
>On Thu, Aug 20, 2026 at 12:14 PM Bradley Morgan <include@xxxxxxxxx> wrote:
>>
>> On 14 August 2026 02:01:53 BST, Bradley Morgan <include@xxxxxxxxx>
>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 <rrobaina@xxxxxxxxxx>
>> >Tested-by: Ricardo Robaina <rrobaina@xxxxxxxxxx>
>> >Signed-off-by: Bradley Morgan <include@xxxxxxxxx>
>> >Link:
>https://lore.kernel.org/r/20260810125726.775689-2-rrobaina@xxxxxxxxxx
>> >---
>> > 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!
>>
>
>Hi Bradley,
>
>Paul will probably explain better, but see the seesion "Don’t get
>discouraged - or impatient" in [1], in the meantime. There was a merge
>window going on recently.
>
>[1]
>https://www.kernel.org/doc/html/latest/process/submitting-patches.html#don-t-get-discouraged-or-impatient
>
>-Ricardo
>
>
Sigh, [1]


This ignoring seems to be a deliberate thing, NOTE THAT THAT WAS MY FIRST
PATCH ever after studying for god knows how long. I have the right to be
annoyed


[1] https://lore.kernel.org/all/20260619130305.27779-2-include@xxxxxxxxx/
Thanks!