Skip to content

Commit

Permalink
fix netfilter conntrack race
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
aojea committed Oct 7, 2024
1 parent e6d6b5c commit fafafae
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 0 deletions.
3 changes: 3 additions & 0 deletions cmd/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ var (
queueID int
metricsBindAddress string
hostnameOverride string
netfilterBug1766Fix bool
)

func init() {
Expand All @@ -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")
Expand Down Expand Up @@ -74,6 +76,7 @@ func main() {
FailOpen: failOpen,
QueueID: queueID,
NodeName: nodeName,
NetfilterBug1766Fix: netfilterBug1766Fix,
}
// creates the in-cluster config
config, err := rest.InClusterConfig()
Expand Down
84 changes: 84 additions & 0 deletions pkg/networkpolicy/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ type Config struct {
BaselineAdminNetworkPolicy bool
QueueID int
NodeName string
NetfilterBug1766Fix bool
}

// NewController returns a new *Controller.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -759,13 +771,85 @@ 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
}
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
Expand Down

0 comments on commit fafafae

Please sign in to comment.