Nftables: Avoid race condition between update and evaluation.

pkg/sentry/socket/netlink/netfilter/protocol.go

@@ -286,7 +286,9 @@ func fillTableInfo(tab *nftables.Table, ms *nlmsg.MessageSet) *syserr.AnnotatedE
 // deleteTable deletes a table for the given family.
 func (p *Protocol) deleteTable(nft *nftables.NFTables, attrs map[uint16]nlmsg.BytesView, family stack.AddressFamily, hdr linux.NetlinkMessageHeader, msgType linux.NfTableMsgType, ms *nlmsg.MessageSet) *syserr.AnnotatedError {
 	if family == stack.Unspec || (!nftables.HasAttr(linux.NFTA_TABLE_NAME, attrs) && !nftables.HasAttr(linux.NFTA_TABLE_HANDLE, attrs)) {
-		nft.Flush(attrs, uint32(ms.PortID))
+		if err := nft.Flush(family, attrs, uint32(ms.PortID)); err != nil {
+			return err
+		}
 		return nil
 	}
 
@@ -728,10 +730,8 @@ func (p *Protocol) deleteChain(nft *nftables.NFTables, attrs map[uint16]nlmsg.By
 		return syserr.NewAnnotatedError(syserr.ErrBusy, fmt.Sprintf("Nftables: Non-recursive delete on a chain with use > 0 is not supported. Chain %s has chain use %d", chain.GetName(), chain.GetChainUse()))
 	}
 
-	// TODO: b/434243967 - Support iteratively deleting rules in a chain to then
-	// delete chains. After deleting all the possible rules, if the chain is
-	// still in use, it cannot be deleted.
-	if chain.GetChainUse() != 0 {
+	// Check if the chain is in use by jumps or goto.
+	if chain.GetChainUse() > 0 {
 		return syserr.NewAnnotatedError(syserr.ErrBusy, fmt.Sprintf("Nftables: Deleting a chain with chain use > 0 is not supported. Chain %s has chain use %d", chain.GetName(), chain.GetChainUse()))
 	}
 
@@ -861,10 +861,6 @@ func (p *Protocol) newRule(nft *nftables.NFTables, st *stack.Stack, attrs map[ui
 		return syserr.NewAnnotatedError(syserr.ErrNotSupported, "Nftables: Hardware offload chains are not supported.")
 	}
 
-	if !chain.IncrementChainUse() {
-		return syserr.NewAnnotatedError(syserr.ErrTooManyOpenFiles, fmt.Sprintf("Nftables: Chain %s has the maximum chain use value at %d", chain.GetName(), chain.GetChainUse()))
-	}
-
 	// TODO - b/434244017: Support replace operations on rules.
 	if msgFlags&linux.NLM_F_REPLACE != 0 {
 		return syserr.NewAnnotatedError(syserr.ErrNotSupported, "Nftables: Replace operations are not currently supported.")
@@ -1087,6 +1083,10 @@ func (p *Protocol) ProcessMessage(ctx context.Context, s *netlink.Socket, msg *n
 
 	msgType := hdr.NetFilterMsgType()
 	st := inet.StackFromContext(ctx).(*netstack.Stack).Stack
+	// Lock the nftables update mutex to prevent concurrent updates.
+	st.LockNFTablesUpdate()
+	defer st.UnlockNFTablesUpdate()
+
 	nft := (st.NFTables()).(*nftables.NFTables)
 	var nfGenMsg linux.NetFilterGenMsg
 
@@ -1106,8 +1106,6 @@ func (p *Protocol) ProcessMessage(ctx context.Context, s *netlink.Socket, msg *n
 	// Nftables functions error check the address family value.
 	family, _ := nftables.AFtoNetlinkAF(nfGenMsg.Family)
 
-	nft.Mu.RLock()
-	defer nft.Mu.RUnlock()
 	switch msgType {
 	case linux.NFT_MSG_GETTABLE:
 		if err := p.getTable(nft, attrs, family, hdr.Flags, ms); err != nil {
@@ -1220,16 +1218,11 @@ func (p *Protocol) processBatchMessage(ctx context.Context, buf []byte, ms *nlms
 	}
 
 	st := inet.StackFromContext(ctx).(*netstack.Stack).Stack
-	nft := (st.NFTables()).(*nftables.NFTables)
-
-	// **********************************************************************
-	// TODO: b/436922484 - Add a transaction system to avoid deep copying the
-	// entire NFTables structure.
-	// Change logic to just replace atomic ptrs.
-	// **********************************************************************
+	// Lock the nftables update mutex to prevent concurrent updates.
+	st.LockNFTablesUpdate()
+	defer st.UnlockNFTablesUpdate()
 
-	nft.Mu.Lock()
-	defer nft.Mu.Unlock()
+	nft := (st.NFTables()).(*nftables.NFTables)
 
 	// No need to hold our own lock
 	nftCopy := nft.DeepCopy()
@@ -1271,7 +1264,10 @@ func (p *Protocol) processBatchMessage(ctx context.Context, buf []byte, ms *nlms
 				if hdr.Flags&linux.NLM_F_ACK != 0 {
 					netlink.DumpAckMessage(hdr, ms)
 				}
-				nft.ReplaceNFTables(nftCopy)
+				// Garbage collect any deleted objects before replacing.
+				nftCopy.GC()
+				// Commit the new nftables instance.
+				commitNftables(st, nftCopy)
 			}
 
 			return nil
@@ -1330,12 +1326,13 @@ func (p *Protocol) processBatchMessage(ctx context.Context, buf []byte, ms *nlms
 			subErr = p.deleteChain(nftCopy, attrs, family, hdr.Flags, hdr.NetFilterMsgType(), ms)
 		case linux.NFT_MSG_NEWRULE:
 			subErr = p.newRule(nftCopy, st, attrs, family, hdr.Flags, ms)
+		case linux.NFT_MSG_DELRULE, linux.NFT_MSG_DESTROYRULE:
+			subErr = nftCopy.DeleteRule(attrs, family, hdr.NetFilterMsgType(), ms)
 		case linux.NFT_MSG_NEWSET:
 			subErr = nftCopy.NewSet(attrs, family, hdr.Flags, ms)
 		case linux.NFT_MSG_NEWSETELEM:
 			subErr = nftCopy.NewSetElements(attrs, family, hdr.Flags, ms)
-		case linux.NFT_MSG_DELRULE, linux.NFT_MSG_DESTROYRULE,
-			linux.NFT_MSG_DELSET, linux.NFT_MSG_DESTROYSET,
+		case linux.NFT_MSG_DELSET, linux.NFT_MSG_DESTROYSET,
 			linux.NFT_MSG_DELSETELEM, linux.NFT_MSG_DESTROYSETELEM,
 			linux.NFT_MSG_NEWOBJ, linux.NFT_MSG_DELOBJ, linux.NFT_MSG_DESTROYOBJ,
 			linux.NFT_MSG_NEWFLOWTABLE, linux.NFT_MSG_DELFLOWTABLE,
@@ -1357,6 +1354,12 @@ func (p *Protocol) processBatchMessage(ctx context.Context, buf []byte, ms *nlms
 	return nil
 }
 
+// commitNftables commits the nftables instance to the stack.
+func commitNftables(st *stack.Stack, nftCopy *nftables.NFTables) {
+	nftCopy.SetGenID(nftCopy.GetGenID() + 1)
+	st.SetNFTables(nftCopy)
+}
+
 // init registers the NETLINK_NETFILTER provider.
 func init() {
 	netlink.RegisterProvider(linux.NETLINK_NETFILTER, NewProtocol)

pkg/tcpip/nftables/BUILD

@@ -1,18 +1,10 @@
-load("//pkg/sync/locking:locking.bzl", "declare_rwmutex")
 load("//tools:defs.bzl", "go_library", "go_test")
 
 package(
     default_applicable_licenses = ["//:license"],
     licenses = ["notice"],
 )
 
-declare_rwmutex(
-    name = "nftables_mutex",
-    out = "nftables_mutex.go",
-    package = "nftables",
-    prefix = "nfTables",
-)
-
 go_library(
     name = "nftables",
     srcs = [
@@ -37,7 +29,6 @@ go_library(
         "nft_route.go",
         "nftables.go",
         "nftables_attrs.go",
-        "nftables_mutex.go",
         "nftables_set.go",
         "nftables_types.go",
         "nftinterp.go",
@@ -53,14 +44,11 @@ go_library(
         "//pkg/marshal/primitive",
         "//pkg/rand",
         "//pkg/sentry/socket/netlink/nlmsg",
-        "//pkg/sync",
-        "//pkg/sync/locking",
         "//pkg/syserr",
         "//pkg/tcpip",
         "//pkg/tcpip/checksum",
         "//pkg/tcpip/header",
         "//pkg/tcpip/stack",
-        "//pkg/tcpip/transport/tcpconntrack",
     ],
 )
 
@@ -73,6 +61,7 @@ go_test(
     library = ":nftables",
     deps = [
         "//pkg/abi/linux",
+        "//pkg/atomicbitops",
         "//pkg/buffer",
         "//pkg/marshal/primitive",
         "//pkg/rand",
@@ -84,5 +73,6 @@ go_test(
         "//pkg/tcpip/header",
         "//pkg/tcpip/stack",
         "@com_github_google_go_cmp//cmp:go_default_library",
+        "@com_github_google_go_cmp//cmp/cmpopts:go_default_library",
     ],
 )

pkg/tcpip/nftables/nft_bitwise.go

@@ -130,6 +130,9 @@ func (op *bitwise) deepCopy() operation {
 // updateReferences implements operation.updateReferences.
 func (op *bitwise) updateReferences(table *Table, sourceTable *Table, sourceOp operation) {}
 
+// destroy implements operation.destroy.
+func (op *bitwise) destroy() {}
+
 // evaluateBitwiseBool performs the bitwise boolean operation on the source register
 // data and stores the result in the destination register.
 func evaluateBitwiseBool(sregBuf, dregBuf, mask, xor []byte) {

pkg/tcpip/nftables/nft_byteorder.go

@@ -104,6 +104,9 @@ func (op *byteorder) deepCopy() operation {
 // updateReferences implements operation.updateReferences.
 func (op *byteorder) updateReferences(table *Table, sourceTable *Table, sourceOp operation) {}
 
+// destroy implements operation.destroy.
+func (op *byteorder) destroy() {}
+
 // evaluate for byteorder performs the byte order operation on the source
 // register and stores the result in the destination register.
 func (op byteorder) evaluate(regs *registerSet, evalCtx opEvalCtx) {

pkg/tcpip/nftables/nft_comparison.go

@@ -94,6 +94,9 @@ func (op *comparison) deepCopy() operation {
 // updateReferences implements operation.updateReferences.
 func (op *comparison) updateReferences(table *Table, sourceTable *Table, sourceOp operation) {}
 
+// destroy implements operation.destroy.
+func (op *comparison) destroy() {}
+
 // evaluate for comparison compares the data in the source register to the given
 // data and breaks from the rule if the comparison is false.
 func (op comparison) evaluate(regs *registerSet, evalCtx opEvalCtx) {

pkg/tcpip/nftables/nft_counter.go

@@ -47,6 +47,9 @@ func (op *counter) deepCopy() operation {
 // updateReferences implements operation.updateReferences.
 func (op *counter) updateReferences(table *Table, sourceTable *Table, sourceOp operation) {}
 
+// destroy implements operation.destroy.
+func (op *counter) destroy() {}
+
 // evaluate for counter increments the counter for the packet and bytes.
 func (op *counter) evaluate(regs *registerSet, evalCtx opEvalCtx) {
 	op.bytes.Add(uint64(evalCtx.pkt.Size()))

pkg/tcpip/nftables/nft_ct.go

@@ -247,6 +247,9 @@ func (op *ctGet) deepCopy() operation {
 // updateReferences implements operation.updateReferences.
 func (op *ctGet) updateReferences(table *Table, sourceTable *Table, sourceOp operation) {}
 
+// destroy implements operation.destroy.
+func (op *ctGet) destroy() {}
+
 // Dump implements operation's Dump interface.
 func (op *ctGet) Dump() ([]byte, *syserr.AnnotatedError) {
 	m := &nlmsg.Message{}
@@ -299,6 +302,9 @@ func (op *ctSet) deepCopy() operation {
 // updateReferences implements operation.updateReferences.
 func (op *ctSet) updateReferences(table *Table, sourceTable *Table, sourceOp operation) {}
 
+// destroy implements operation.destroy.
+func (op *ctSet) destroy() {}
+
 // initCTSet initializes a ct set operation.
 func initCTSet(tab *Table, sreg uint8, attrs map[uint16]nlmsg.BytesView) (*ctSet, *syserr.AnnotatedError) {
 	return nil, syserr.NewAnnotatedError(syserr.ErrNotSupported, "ct set operation is not supported")

pkg/tcpip/nftables/nft_fib.go

@@ -337,6 +337,9 @@ func (op *fib) Dump() ([]byte, *syserr.AnnotatedError) {
 	return m.Buffer(), nil
 }
 
+// destroy implements operation.destroy.
+func (op *fib) destroy() {}
+
 // deepCopy implements operation's deepCopy interface.
 func (op *fib) deepCopy() operation {
 	opCopy := &fib{}

pkg/tcpip/nftables/nft_immediate.go

@@ -130,6 +130,17 @@ func (op *immediate) updateReferences(table *Table, sourceTable *Table, sourceOp
 	}
 }
 
+// destroy implements operation.destroy.
+func (op *immediate) destroy() {
+	v := &op.verdict
+	if v.Chain == nil {
+		return
+	}
+	chain := v.Chain
+	chain.DecrementChainUse()
+	v.Chain = nil
+}
+
 // checkCompatibility implements operation.checkCompatibility.
 func (op immediate) checkCompatibility(cCtx *opCompatCtx) *syserr.AnnotatedError {
 	return nil

pkg/tcpip/nftables/nft_last.go

@@ -49,6 +49,9 @@ func (op *last) deepCopy() operation {
 // updateReferences implements operation.updateReferences.
 func (op *last) updateReferences(table *Table, sourceTable *Table, sourceOp operation) {}
 
+// destroy implements operation.destroy.
+func (op *last) destroy() {}
+
 // evaluate for last records the last time the operation was evaluated and flags
 // if this was the first time the operation was evaluated.
 func (op *last) evaluate(regs *registerSet, evalCtx opEvalCtx) {

pkg/tcpip/nftables/nft_lookup.go

@@ -16,6 +16,7 @@ package nftables
 
 import (
 	"fmt"
+	"slices"
 
 	"gvisor.dev/gvisor/pkg/abi/linux"
 	"gvisor.dev/gvisor/pkg/sentry/socket/netlink/nlmsg"
@@ -118,6 +119,18 @@ func (op *lookupOp) updateReferences(table *Table, sourceTable *Table, sourceOp
 	op.set.bindings = append(op.set.bindings, op)
 }
 
+// destroy implements operation.destroy.
+func (op *lookupOp) destroy() {
+	if op.set == nil {
+		return
+	}
+	// Remove the lookup operation from the set bindings.
+	op.set.bindings = slices.DeleteFunc(op.set.bindings, func(b *lookupOp) bool {
+		return b == op
+	})
+	op.set = nil
+}
+
 // checkCompatibility implements operation.checkCompatibility.
 func (op *lookupOp) checkCompatibility(cCtx *opCompatCtx) *syserr.AnnotatedError {
 	return nil

pkg/tcpip/nftables/nft_masq.go

@@ -137,6 +137,9 @@ func (m *masqOp) deepCopy() operation {
 // updateReferences implements operation.updateReferences.
 func (m *masqOp) updateReferences(table *Table, sourceTable *Table, sourceOp operation) {}
 
+// destroy implements operation.destroy.
+func (m *masqOp) destroy() {}
+
 // Dump implements operation.Dump.
 func (m *masqOp) Dump() ([]byte, *syserr.AnnotatedError) {
 	msg := &nlmsg.Message{}

pkg/tcpip/nftables/nft_metaload.go

@@ -68,6 +68,9 @@ func (op *metaLoad) deepCopy() operation {
 // updateReferences implements operation.updateReferences.
 func (op *metaLoad) updateReferences(table *Table, sourceTable *Table, sourceOp operation) {}
 
+// destroy implements operation.destroy.
+func (op *metaLoad) destroy() {}
+
 // evaluate for MetaLoad loads specific meta data into the destination register.
 func (op metaLoad) evaluate(regs *registerSet, evalCtx opEvalCtx) {
 	var target []byte

pkg/tcpip/nftables/nft_metaset.go

@@ -56,6 +56,9 @@ func (op *metaSet) deepCopy() operation {
 // updateReferences implements operation.updateReferences.
 func (op *metaSet) updateReferences(table *Table, sourceTable *Table, sourceOp operation) {}
 
+// destroy implements operation.destroy.
+func (op *metaSet) destroy() {}
+
 // evaluate for metaSet sets specific meta data to the value in the source
 // register.
 func (op metaSet) evaluate(regs *registerSet, evalCtx opEvalCtx) {

pkg/tcpip/nftables/nft_nat.go

@@ -102,6 +102,9 @@ func (n *natOp) deepCopy() operation {
 // updateReferences implements operation.updateReferences.
 func (n *natOp) updateReferences(table *Table, sourceTable *Table, sourceOp operation) {}
 
+// destroy implements operation.destroy.
+func (n *natOp) destroy() {}
+
 // nfNatRange is the equivalent of struct nf_nat_range2 in Linux.
 type nfNatRange struct {
 	minAddr  tcpip.Address

pkg/tcpip/nftables/nft_payload_load.go

@@ -90,6 +90,9 @@ func (op *payloadLoad) deepCopy() operation {
 // updateReferences implements operation.updateReferences.
 func (op *payloadLoad) updateReferences(table *Table, sourceTable *Table, sourceOp operation) {}
 
+// destroy implements operation.destroy.
+func (op *payloadLoad) destroy() {}
+
 // evaluate for PayloadLoad loads data from the packet payload into the
 // destination register.
 func (op payloadLoad) evaluate(regs *registerSet, evalCtx opEvalCtx) {

pkg/tcpip/nftables/nft_payload_set.go

@@ -101,6 +101,9 @@ func (op *payloadSet) deepCopy() operation {
 // updateReferences implements operation.updateReferences.
 func (op *payloadSet) updateReferences(table *Table, sourceTable *Table, sourceOp operation) {}
 
+// destroy implements operation.destroy.
+func (op *payloadSet) destroy() {}
+
 // evaluate for PayloadSet sets data in the packet payload to the value in the
 // source register.
 func (op payloadSet) evaluate(regs *registerSet, evalCtx opEvalCtx) {

pkg/tcpip/nftables/nft_ranged.go

@@ -103,6 +103,9 @@ func (op *ranged) deepCopy() operation {
 // updateReferences implements operation.updateReferences.
 func (op *ranged) updateReferences(table *Table, sourceTable *Table, sourceOp operation) {}
 
+// destroy implements operation.destroy.
+func (op *ranged) destroy() {}
+
 // evaluate for Ranged checks whether the source register data is within the
 // specified inclusive range and breaks from the rule if comparison is false.
 func (op ranged) evaluate(regs *registerSet, evalCtx opEvalCtx) {

pkg/tcpip/nftables/nft_route.go

@@ -115,6 +115,9 @@ func (op *route) deepCopy() operation {
 // updateReferences implements operation.updateReferences.
 func (op *route) updateReferences(table *Table, sourceTable *Table, sourceOp operation) {}
 
+// destroy implements operation.destroy.
+func (op *route) destroy() {}
+
 // evaluate for Route loads specific routing data into the destination register.
 func (op route) evaluate(regs *registerSet, evalCtx opEvalCtx) {
 	// Gets the target data to be stored in the destination register.