On Fri, Nov 7, 2025 at 7:46 PM Paul Moore <[email protected]> wrote: > > On Nov 6, 2025 Ricardo Robaina <[email protected]> wrote: > > > > Netfilter code (net/netfilter/nft_log.c and net/netfilter/xt_AUDIT.c) > > have to be kept in sync. Both source files had duplicated versions of > > audit_ip4() and audit_ip6() functions, which can result in lack of > > consistency and/or duplicated work. > > > > This patch adds two helper functions in audit.c that can be called by > > netfilter code commonly, aiming to improve maintainability and > > consistency. > > > > Signed-off-by: Ricardo Robaina <[email protected]> > > Acked-by: Florian Westphal <[email protected]> > > --- > > include/linux/audit.h | 12 +++++++++++ > > kernel/audit.c | 39 ++++++++++++++++++++++++++++++++++++ > > net/netfilter/nft_log.c | 43 ++++------------------------------------ > > net/netfilter/xt_AUDIT.c | 43 ++++------------------------------------ > > 4 files changed, 59 insertions(+), 78 deletions(-) > > ... > > > diff --git a/net/netfilter/nft_log.c b/net/netfilter/nft_log.c > > index e35588137995..f53fb4222134 100644 > > --- a/net/netfilter/nft_log.c > > +++ b/net/netfilter/nft_log.c > > @@ -26,41 +26,6 @@ struct nft_log { > > char *prefix; > > }; > > > > -static bool audit_ip4(struct audit_buffer *ab, struct sk_buff *skb) > > -{ > > - struct iphdr _iph; > > - const struct iphdr *ih; > > - > > - ih = skb_header_pointer(skb, skb_network_offset(skb), sizeof(_iph), > > &_iph); > > - if (!ih) > > - return false; > > - > > - audit_log_format(ab, " saddr=%pI4 daddr=%pI4 proto=%hhu", > > - &ih->saddr, &ih->daddr, ih->protocol); > > - > > - return true; > > -} > > - > > -static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb) > > -{ > > - struct ipv6hdr _ip6h; > > - const struct ipv6hdr *ih; > > - u8 nexthdr; > > - __be16 frag_off; > > - > > - ih = skb_header_pointer(skb, skb_network_offset(skb), sizeof(_ip6h), > > &_ip6h); > > - if (!ih) > > - return false; > > - > > - nexthdr = ih->nexthdr; > > - ipv6_skip_exthdr(skb, skb_network_offset(skb) + sizeof(_ip6h), > > &nexthdr, &frag_off); > > - > > - audit_log_format(ab, " saddr=%pI6c daddr=%pI6c proto=%hhu", > > - &ih->saddr, &ih->daddr, nexthdr); > > - > > - return true; > > -} > > - > > static void nft_log_eval_audit(const struct nft_pktinfo *pkt) > > { > > struct sk_buff *skb = pkt->skb; > > @@ -80,18 +45,18 @@ static void nft_log_eval_audit(const struct nft_pktinfo > > *pkt) > > case NFPROTO_BRIDGE: > > switch (eth_hdr(skb)->h_proto) { > > case htons(ETH_P_IP): > > - fam = audit_ip4(ab, skb) ? NFPROTO_IPV4 : -1; > > + fam = audit_log_packet_ip4(ab, skb) ? NFPROTO_IPV4 : > > -1; > > break; > > case htons(ETH_P_IPV6): > > - fam = audit_ip6(ab, skb) ? NFPROTO_IPV6 : -1; > > + fam = audit_log_packet_ip6(ab, skb) ? NFPROTO_IPV6 : > > -1; > > break; > > } > > break; > > case NFPROTO_IPV4: > > - fam = audit_ip4(ab, skb) ? NFPROTO_IPV4 : -1; > > + fam = audit_log_packet_ip4(ab, skb) ? NFPROTO_IPV4 : -1; > > break; > > case NFPROTO_IPV6: > > - fam = audit_ip6(ab, skb) ? NFPROTO_IPV6 : -1; > > + fam = audit_log_packet_ip6(ab, skb) ? NFPROTO_IPV6 : -1; > > break; > > } > > We can probably take this a step further by moving the case statements > into the audit functions too. I think this will make some of the other > changes a bit cleaner and should reduce the amount of audit code in the > NFT code. > > If we don't want to do that, it might be worthwhile to take the > NFPROTO_BRIDGE protocol family reset shown below in audit_log_nft_skb() > and use that in the nft_log_eval_audit() function so we aren't > duplicating calls into the audit code. > > [WARNING: completely untested code, but you should get the basic idea] > > diff --git a/kernel/audit.c b/kernel/audit.c > index 26a332ffb1b8..72ba3f51f859 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -2538,6 +2538,59 @@ static void audit_log_set_loginuid(kuid_t > koldloginuid, kuid_t kloginuid, > audit_log_end(ab); > } > > +int audit_log_nft_skb(struct audit_buffer *ab, > + struct sk_buff *skb, u8 nfproto) > +{ > + /* find the IP protocol in the case of NFPROTO_BRIDGE */ > + if (nfproto == NFPROTO_BRIDGE) { > + switch (eth_hdr(skb)->h_proto) { > + case htons(ETH_P_IP): > + nfproto = NFPROTO_IPV4; > + case htons(ETH_P_IPV6): > + nfproto = NFPROTO_IPV6; > + default: > + goto unknown_proto; > + } > + } > + > + switch (nfproto) { > + case NFPROTO_IPV4: { > + struct iphdr iph; > + const struct iphdr *ih; > + > + ih = skb_header_pointer(skb, skb_network_offset(skb), > + sizeof(_iph), &_iph); > + if (!ih) > + return -ENOMEM; > + > + audit_log_format(ab, " saddr=%pI4 daddr=%pI4 proto=%hhu", > + &ih->saddr, &ih->daddr, ih->protocol); > + break; > + } > + case NFPROTO_IPV6: { > + struct ipv6hdr iph; > + const struct ipv6hdr *ih; > + > + ih = skb_header_pointer(skb, skb_network_offset(skb), > + sizeof(_iph), &_iph); > + if (!ih) > + return -ENOMEM; > + > + audit_log_format(ab, " saddr=%pI6 daddr=%pI6 proto=%hhu", > + &ih->saddr, &ih->daddr, ih->protocol); > + break; > + } > + default: > + goto unknown_proto; > + } > + > + return 0; > + > +unknown_proto: > + audit_log_format(ab, " saddr=? daddr=? proto=?"); > + return -EPFNOSUPPORT; > +} > + > /** > * audit_set_loginuid - set current task's loginuid > * @loginuid: loginuid value > diff --git a/net/netfilter/nft_log.c b/net/netfilter/nft_log.c > index e35588137995..6f444e2ad70a 100644 > --- a/net/netfilter/nft_log.c > +++ b/net/netfilter/nft_log.c > @@ -75,28 +75,7 @@ static void nft_log_eval_audit(const struct nft_pktinfo > *pkt) > return; > > audit_log_format(ab, "mark=%#x", skb->mark); > - > - switch (nft_pf(pkt)) { > - case NFPROTO_BRIDGE: > - switch (eth_hdr(skb)->h_proto) { > - case htons(ETH_P_IP): > - fam = audit_ip4(ab, skb) ? NFPROTO_IPV4 : -1; > - break; > - case htons(ETH_P_IPV6): > - fam = audit_ip6(ab, skb) ? NFPROTO_IPV6 : -1; > - break; > - } > - break; > - case NFPROTO_IPV4: > - fam = audit_ip4(ab, skb) ? NFPROTO_IPV4 : -1; > - break; > - case NFPROTO_IPV6: > - fam = audit_ip6(ab, skb) ? NFPROTO_IPV6 : -1; > - break; > - } > - > - if (fam == -1) > - audit_log_format(ab, " saddr=? daddr=? proto=-1"); > + audit_log_nft_skb(ab, skb, nft_pf(pkt)); > > audit_log_end(ab); > } > > -- > paul-moore.com >
Thanks for reviewing this patch, Paul. It makes sense to me. I'll work on a newer version addressing your suggestions.
