Thanks for reviewing this patch, Paul! Sounds great, I'll work on the v3 shortly.
On Thu, Sep 25, 2025 at 5:41 PM Paul Moore <[email protected]> wrote: > > On Sep 25, 2025 Ricardo Robaina <[email protected]> wrote: > > > > NETFILTER_PKT records show both source and destination > > addresses, in addition to the associated networking protocol. > > However, it lacks the ports information, which is often > > valuable for troubleshooting. > > > > This patch adds both source and destination port numbers, > > 'sport' and 'dport' respectively, to TCP, UDP, UDP-Lite and > > SCTP-related NETFILTER_PKT records. > > > > type=NETFILTER_PKT ... saddr=127.0.0.1 daddr=127.0.0.1 proto=icmp > > type=NETFILTER_PKT ... saddr=::1 daddr=::1 proto=ipv6-icmp > > type=NETFILTER_PKT ... daddr=127.0.0.1 proto=udp sport=38173 dport=42424 > > type=NETFILTER_PKT ... daddr=::1 proto=udp sport=56852 dport=42424 > > type=NETFILTER_PKT ... daddr=127.0.0.1 proto=tcp sport=57022 dport=42424 > > type=NETFILTER_PKT ... daddr=::1 proto=tcp sport=50810 dport=42424 > > type=NETFILTER_PKT ... daddr=127.0.0.1 proto=sctp sport=54944 dport=42424 > > type=NETFILTER_PKT ... daddr=::1 proto=sctp sport=57963 dport=42424 > > > > Link: https://github.com/linux-audit/audit-kernel/issues/162 > > Signed-off-by: Ricardo Robaina <[email protected]> > > --- > > net/netfilter/xt_AUDIT.c | 42 +++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 41 insertions(+), 1 deletion(-) > > > > diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c > > index b6a015aee0ce..9fc8a5429fa9 100644 > > --- a/net/netfilter/xt_AUDIT.c > > +++ b/net/netfilter/xt_AUDIT.c > > @@ -19,6 +19,7 @@ > > #include <linux/netfilter_bridge/ebtables.h> > > #include <net/ipv6.h> > > #include <net/ip.h> > > +#include <linux/sctp.h> > > > > MODULE_LICENSE("GPL"); > > MODULE_AUTHOR("Thomas Graf <[email protected]>"); > > @@ -32,6 +33,7 @@ static bool audit_ip4(struct audit_buffer *ab, struct > > sk_buff *skb) > > { > > struct iphdr _iph; > > const struct iphdr *ih; > > + __be16 dport, sport; > > > > ih = skb_header_pointer(skb, skb_network_offset(skb), sizeof(_iph), > > &_iph); > > if (!ih) > > @@ -40,6 +42,25 @@ static bool audit_ip4(struct audit_buffer *ab, struct > > sk_buff *skb) > > audit_log_format(ab, " saddr=%pI4 daddr=%pI4 proto=%hhu", > > &ih->saddr, &ih->daddr, ih->protocol); > > > > + switch (ih->protocol) { > > + case IPPROTO_TCP: > > + sport = tcp_hdr(skb)->source; > > + dport = tcp_hdr(skb)->dest; > > + break; > > + case IPPROTO_UDP: > > + case IPPROTO_UDPLITE: > > + sport = udp_hdr(skb)->source; > > + dport = udp_hdr(skb)->dest; > > + break; > > + case IPPROTO_SCTP: > > + sport = sctp_hdr(skb)->source; > > + dport = sctp_hdr(skb)->dest; > > + } > > + > > + if (ih->protocol == IPPROTO_TCP || ih->protocol == IPPROTO_UDP || > > + ih->protocol == IPPROTO_UDPLITE || ih->protocol == IPPROTO_SCTP) > > + audit_log_format(ab, " sport=%hu dport=%hu", ntohs(sport), > > ntohs(dport)); > > return true; > > } > > Instead of having the switch statement and then doing an additional if > statement, why not fold it all into the switch statement? Yes, you > would have multiple audit_log_format() calls, but they are trivial to > cut-n-paste, and it saves the extra per-packet checking at runtime. > > switch (ih->protocol) { > case IPPROTO_TCP: > audit_log_format(ab, " sport=...", > tcp_hdr(skb)->source, > tcp_hdr(skb)->dest); > break; > ... > } > > ... considering how expensive multiple audit_log_format() calls can be, > it might even be worth considering consolidating the two calls into one: > > switch (ih->protocol) { > case IPPROTO_TCP: > audit_log_format(ab, " saddr=...", > ih->saddr, > ... > tcp_hdr(skb)->source, > tcp_hdr(skb)->dest); > break; > ... > default: > audit_log_format(ab, " saddr=...", > ih->saddr, > ...); > } > > -- > paul-moore.com >
