From fafafaebb00e92e81a913cf340c8ad686d3ce79c Mon Sep 17 00:00:00 2001
From: Antonio Ojea <aojea@google.com>
Date: Mon, 7 Oct 2024 07:17:04 +0000
Subject: [PATCH] fix netfilter conntrack race

This was fixed in the kernel in 6.12 (commit 8af79d3edb5f)

However, users need a workaround to avoid hitting this bugs in existing
kernels.
Add a feature flag enabled by default to implement the workaround, that
consists in processing the DNS packets on the prerouting hook, after
dnat happens, so we can have the resolved IPs of the DNS server,
and avoid to process them in the postrouting hook.
---
 cmd/main.go                     |  3 ++
 pkg/networkpolicy/controller.go | 84 +++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/cmd/main.go b/cmd/main.go
index 1ad27d9..9bf1485 100644
--- a/cmd/main.go
+++ b/cmd/main.go
@@ -34,6 +34,7 @@ var (
 	queueID                    int
 	metricsBindAddress         string
 	hostnameOverride           string
+	netfilterBug1766Fix        bool
 )
 
 func init() {
@@ -43,6 +44,7 @@ func init() {
 	flag.IntVar(&queueID, "nfqueue-id", 100, "Number of the nfqueue used")
 	flag.StringVar(&metricsBindAddress, "metrics-bind-address", ":9080", "The IP address and port for the metrics server to serve on")
 	flag.StringVar(&hostnameOverride, "hostname-override", "", "If non-empty, will be used as the name of the Node that kube-network-policies is running on. If unset, the node name is assumed to be the same as the node's hostname.")
+	flag.BoolVar(&netfilterBug1766Fix, "netfilter-bug-1766-fix", true, "If set, process DNS packets on the PREROUTING hooks to avoid the race on the conntrack subsystem, not needed for kernels 6.12+ (see https://bugzilla.netfilter.org/show_bug.cgi?id=1766)")
 
 	flag.Usage = func() {
 		fmt.Fprint(os.Stderr, "Usage: kube-network-policies [options]\n\n")
@@ -74,6 +76,7 @@ func main() {
 		FailOpen:                   failOpen,
 		QueueID:                    queueID,
 		NodeName:                   nodeName,
+		NetfilterBug1766Fix:        netfilterBug1766Fix,
 	}
 	// creates the in-cluster config
 	config, err := rest.InClusterConfig()
diff --git a/pkg/networkpolicy/controller.go b/pkg/networkpolicy/controller.go
index d35b213..20d453b 100644
--- a/pkg/networkpolicy/controller.go
+++ b/pkg/networkpolicy/controller.go
@@ -66,6 +66,7 @@ type Config struct {
 	BaselineAdminNetworkPolicy bool
 	QueueID                    int
 	NodeName                   string
+	NetfilterBug1766Fix        bool
 }
 
 // NewController returns a new *Controller.
@@ -692,6 +693,17 @@ func (c *Controller) syncNFTablesRules(ctx context.Context) error {
 	tx.Flush(&knftables.Chain{
 		Name: chainName,
 	})
+
+	// DNS is processed by addDNSRacersWorkaroundRules()
+	// TODO: remove once kernel fix is on most distros
+	if c.config.NetfilterBug1766Fix {
+		tx.Add(&knftables.Rule{
+			Chain:   chainName,
+			Rule:    "udp dport 53 accept",
+			Comment: ptr.To("process DNS traffic on PREROUTING hook with network policy enforcement to avoid netfiler race bug"),
+		})
+	}
+
 	// IPv6 needs ICMP Neighbor Discovery to work
 	tx.Add(&knftables.Rule{
 		Chain: chainName,
@@ -759,6 +771,10 @@ func (c *Controller) syncNFTablesRules(ctx context.Context) error {
 		})
 	}
 
+	if c.config.NetfilterBug1766Fix {
+		c.addDNSRacersWorkaroundRules(tx)
+	}
+
 	if err := c.nft.Run(ctx, tx); err != nil {
 		klog.Infof("error syncing nftables rules %v", err)
 		return err
@@ -766,6 +782,74 @@ func (c *Controller) syncNFTablesRules(ctx context.Context) error {
 	return nil
 }
 
+// To avoid a kernel bug caused by UDP DNS request racing with conntrack
+// process the DNS packets only on the PREROUTING hook after DNAT happens
+// so we can see the resolved destination IPs, typically the ones of the Pods
+// that are used for the Kubernetes DNS Service.
+// xref: https://github.com/kubernetes-sigs/kube-network-policies/issues/12
+// This can be removed once all kernels contain the fix in
+// https://github.com/torvalds/linux/commit/8af79d3edb5fd2dce35ea0a71595b6d4f9962350
+// TODO: remove once kernel fix is on most distros
+func (c *Controller) addDNSRacersWorkaroundRules(tx *knftables.Transaction) {
+	hook := knftables.PreroutingHook
+	chainName := string(hook)
+	tx.Add(&knftables.Chain{
+		Name:     chainName,
+		Type:     knftables.PtrTo(knftables.FilterType),
+		Hook:     knftables.PtrTo(hook),
+		Priority: knftables.PtrTo(knftables.DNATPriority + "+5"),
+	})
+	tx.Flush(&knftables.Chain{
+		Name: chainName,
+	})
+
+	action := fmt.Sprintf("queue num %d", c.config.QueueID)
+	if c.config.FailOpen {
+		action += " bypass"
+	}
+
+	if !c.config.AdminNetworkPolicy && !c.config.BaselineAdminNetworkPolicy {
+		tx.Add(&knftables.Rule{
+			Chain: chainName,
+			Rule: knftables.Concat(
+				"ip", "saddr", "@", podV4IPsSet, "udp dport 53", action,
+			),
+			Comment: ptr.To("process IPv4 traffic destined to a DNS server with network policy enforcement"),
+		})
+
+		tx.Add(&knftables.Rule{
+			Chain: chainName,
+			Rule: knftables.Concat(
+				"ip", "daddr", "@", podV4IPsSet, "udp dport 53", action,
+			),
+			Comment: ptr.To("process IPv4 traffic destined to a DNS server with network policy enforcement"),
+		})
+
+		tx.Add(&knftables.Rule{
+			Chain: chainName,
+			Rule: knftables.Concat(
+				"ip6", "saddr", "@", podV6IPsSet, "udp dport 53", action,
+			),
+			Comment: ptr.To("process IPv6 traffic destined to a DNS serverwith network policy enforcement"),
+		})
+
+		tx.Add(&knftables.Rule{
+			Chain: chainName,
+			Rule: knftables.Concat(
+				"ip6", "daddr", "@", podV6IPsSet, "udp dport 53", action,
+			),
+			Comment: ptr.To("process IPv6 traffic destined to a DNS serverwith network policy enforcement"),
+		})
+	} else {
+		tx.Add(&knftables.Rule{
+			Chain: chainName,
+			Rule: knftables.Concat(
+				"udp dport 53", action,
+			),
+		})
+	}
+}
+
 func (c *Controller) cleanNFTablesRules() {
 	tx := c.nft.NewTransaction()
 	// Add+Delete is idempotent and won't return an error if the table doesn't already