mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH] audit: move the nlmsg_len fixup from __audit_log_end() to send time
@ 2026-08-14  1:01 Bradley Morgan
  2026-08-20 15:14 ` Bradley Morgan
  2026-08-25  0:08 ` Paul Moore
  0 siblings, 2 replies; 7+ messages in thread
From: Bradley Morgan @ 2026-08-14  1:01 UTC (permalink / raw)
  To: Paul Moore; +Cc: Eric Paris, Ricardo Robaina, audit, linux-kernel, include

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@redhat.com>
Tested-by: Ricardo Robaina <rrobaina@redhat.com>
Signed-off-by: Bradley Morgan <include@grrlz.net>
Link: https://lore.kernel.org/r/20260810125726.775689-2-rrobaina@redhat.com
---
 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);
-- 
2.47.3


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] audit: move the nlmsg_len fixup from __audit_log_end() to send time
  2026-08-14  1:01 [PATCH] audit: move the nlmsg_len fixup from __audit_log_end() to send time Bradley Morgan
@ 2026-08-20 15:14 ` Bradley Morgan
  2026-08-20 15:57   ` Ricardo Robaina
  2026-08-25  0:08 ` Paul Moore
  1 sibling, 1 reply; 7+ messages in thread
From: Bradley Morgan @ 2026-08-20 15:14 UTC (permalink / raw)
  To: Paul Moore; +Cc: Eric Paris, Ricardo Robaina, audit, linux-kernel, akpm

On 14 August 2026 02:01:53 BST, Bradley Morgan <include@grrlz.net> 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@redhat.com>
>Tested-by: Ricardo Robaina <rrobaina@redhat.com>
>Signed-off-by: Bradley Morgan <include@grrlz.net>
>Link: https://lore.kernel.org/r/20260810125726.775689-2-rrobaina@redhat.com
>---
> 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!

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] audit: move the nlmsg_len fixup from __audit_log_end() to send time
  2026-08-20 15:14 ` Bradley Morgan
@ 2026-08-20 15:57   ` Ricardo Robaina
  2026-08-20 16:38     ` Bradley Morgan
  0 siblings, 1 reply; 7+ messages in thread
From: Ricardo Robaina @ 2026-08-20 15:57 UTC (permalink / raw)
  To: Bradley Morgan; +Cc: Paul Moore, Eric Paris, audit, linux-kernel, akpm

On Thu, Aug 20, 2026 at 12:14 PM Bradley Morgan <include@grrlz.net> wrote:
>
> On 14 August 2026 02:01:53 BST, Bradley Morgan <include@grrlz.net> 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@redhat.com>
> >Tested-by: Ricardo Robaina <rrobaina@redhat.com>
> >Signed-off-by: Bradley Morgan <include@grrlz.net>
> >Link: https://lore.kernel.org/r/20260810125726.775689-2-rrobaina@redhat.com
> >---
> > 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] audit: move the nlmsg_len fixup from __audit_log_end() to send time
  2026-08-20 15:57   ` Ricardo Robaina
@ 2026-08-20 16:38     ` Bradley Morgan
  2026-08-25  1:28       ` Paul Moore
  0 siblings, 1 reply; 7+ messages in thread
From: Bradley Morgan @ 2026-08-20 16:38 UTC (permalink / raw)
  To: Ricardo Robaina; +Cc: Paul Moore, Eric Paris, audit, linux-kernel, akpm

On 20 August 2026 16:57:12 BST, Ricardo Robaina <rrobaina@redhat.com>
wrote:
>On Thu, Aug 20, 2026 at 12:14 PM Bradley Morgan <include@grrlz.net> wrote:
>>
>> On 14 August 2026 02:01:53 BST, Bradley Morgan <include@grrlz.net>
>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@redhat.com>
>> >Tested-by: Ricardo Robaina <rrobaina@redhat.com>
>> >Signed-off-by: Bradley Morgan <include@grrlz.net>
>> >Link:
>https://lore.kernel.org/r/20260810125726.775689-2-rrobaina@redhat.com
>> >---
>> > 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@grrlz.net/
Thanks!

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] audit: move the nlmsg_len fixup from __audit_log_end() to  send time
  2026-08-14  1:01 [PATCH] audit: move the nlmsg_len fixup from __audit_log_end() to send time Bradley Morgan
  2026-08-20 15:14 ` Bradley Morgan
@ 2026-08-25  0:08 ` Paul Moore
  2026-08-25  0:15   ` Bradley Morgan
  1 sibling, 1 reply; 7+ messages in thread
From: Paul Moore @ 2026-08-25  0:08 UTC (permalink / raw)
  To: Bradley Morgan; +Cc: Eric Paris, Ricardo Robaina, audit, linux-kernel, include

On Aug 13, 2026 Bradley Morgan <include@grrlz.net> 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@redhat.com>
> Tested-by: Ricardo Robaina <rrobaina@redhat.com>
> Signed-off-by: Bradley Morgan <include@grrlz.net>
> Link: https://lore.kernel.org/r/20260810125726.775689-2-rrobaina@redhat.com
> ---
>  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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] audit: move the nlmsg_len fixup from __audit_log_end() to  send time
  2026-08-25  0:08 ` Paul Moore
@ 2026-08-25  0:15   ` Bradley Morgan
  0 siblings, 0 replies; 7+ messages in thread
From: Bradley Morgan @ 2026-08-25  0:15 UTC (permalink / raw)
  To: Paul Moore; +Cc: Eric Paris, Ricardo Robaina, audit, linux-kernel

On 25 August 2026 01:08:46 BST, Paul Moore <paul@paul-moore.com> wrote:
>On Aug 13, 2026 Bradley Morgan <include@grrlz.net> 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@redhat.com>
>> Tested-by: Ricardo Robaina <rrobaina@redhat.com>
>> Signed-off-by: Bradley Morgan <include@grrlz.net>
>> Link:
>https://lore.kernel.org/r/20260810125726.775689-2-rrobaina@redhat.com
>> ---
>>  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.

Hmm, interesting, yeah that works.


>
>>  		/* can we send to anyone via unicast? */
>>  		if (!sk) {
>>  			if (err_hook)
>
>--
>paul-moore.com
>

Thanks!

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] audit: move the nlmsg_len fixup from __audit_log_end() to send time
  2026-08-20 16:38     ` Bradley Morgan
@ 2026-08-25  1:28       ` Paul Moore
  0 siblings, 0 replies; 7+ messages in thread
From: Paul Moore @ 2026-08-25  1:28 UTC (permalink / raw)
  To: Bradley Morgan; +Cc: Ricardo Robaina, Eric Paris, audit, linux-kernel, akpm

On Thu, Aug 20, 2026 at 12:38 PM Bradley Morgan <include@grrlz.net> wrote:
> On 20 August 2026 16:57:12 BST, Ricardo Robaina <rrobaina@redhat.com>
> wrote:

...

> >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.
>
> 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

You've already seen my feedback on this patch, and it sounds like we
are in agreement as to why it would be a step backward, but I wanted
to comment quickly on this other aspect of the thread.

As Ricardo already pointed out, this patch was posted during an
awkward period when the audit/dev branch was only open to critical
fixes.  If you haven't already seen it via the audit MAINTAINERS
entry, we have a document describing how the audit tree is managed and
some of the basic processes, a link is below:

https://github.com/linux-audit/audit-kernel/blob/main/README.md

It's also important to note that many Open Source maintainers have
been under a lot of pressure lately due to a significant increase in
reported bugs/vulnerabilities from LLM-based bug hunters.  For obvious
reasons, not all of these reports are public, but I think it's safe to
say that any maintainer of software with more than a few users is
dealing with these reports.

--
paul-moore.com

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-08-25  1:28 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-08-14  1:01 [PATCH] audit: move the nlmsg_len fixup from __audit_log_end() to send time Bradley Morgan
2026-08-20 15:14 ` Bradley Morgan
2026-08-20 15:57   ` Ricardo Robaina
2026-08-20 16:38     ` Bradley Morgan
2026-08-25  1:28       ` Paul Moore
2026-08-25  0:08 ` Paul Moore
2026-08-25  0:15   ` Bradley Morgan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®