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

[Paul Moore]

On Aug 13, 2026 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;

One of the reasons we set the length in __audit_log_end() is so that we
only do it once, plus the multicast fixup.  In this patch we still set the
length in __audit_log_end() as well as potentially multiple times in
kauditd_send_queue().  This patch does drop the multicast fixup, but it
isn't as clean a solution conceptually as the current code in my opinion.

Ultimately, in this patch we still have at least two length calculations
with the potential for additional unnecessary calculations/assignments if
we need to loop through kauditd_send_queue() multiple times.  The existing
code is capped at two calculations/assignments.  I appreciate the time
you've put into this, but I think the existing code is the better option
at this point in time.

>  		/* can we send to anyone via unicast? */
>  		if (!sk) {
>  			if (err_hook)

--
paul-moore.com