Automated rollback of changelist 580051079

PiperOrigin-RevId: 580649803
google · Nov 8, 2023 · 40ee36a · 40ee36a
1 parent 9284335
commit 40ee36a
Show file tree

Hide file tree

Showing 13 changed files with 90 additions and 172 deletions.
diff --git a/pkg/sentry/socket/netfilter/BUILD b/pkg/sentry/socket/netfilter/BUILD
@@ -38,14 +38,11 @@ go_library(
 )
 
 go_test(
-    name = "netfilter_x_test",
-    srcs = ["netfilter_x_test.go"],
-    embedsrcs = [
-        "accept_blob",
-        "istio_blob",
-    ],
+    name = "netfilter_test",
+    srcs = ["netfilter_test.go"],
+    embedsrcs = ["istio_blob"],
+    library = ":netfilter",
     deps = [
-        ":netfilter",
         "//pkg/sentry/kernel/auth",
         "//pkg/tcpip/stack",
     ],

diff --git a/pkg/sentry/socket/netfilter/accept_blob b/pkg/sentry/socket/netfilter/accept_blob
diff --git a/...ntry/socket/netfilter/netfilter_x_test.go → ...sentry/socket/netfilter/netfilter_test.go b/...ntry/socket/netfilter/netfilter_x_test.go → ...sentry/socket/netfilter/netfilter_test.go
@@ -12,17 +12,26 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package netfilter_test
+package netfilter
 
 import (
 	_ "embed"
 	"testing"
 
 	"gvisor.dev/gvisor/pkg/sentry/kernel/auth"
-	"gvisor.dev/gvisor/pkg/sentry/socket/netfilter"
 	"gvisor.dev/gvisor/pkg/tcpip/stack"
 )
 
+// istioBlob is a golden iptables ruleset generated by Istio. It is already in
+// the format ready to by passed to IPT_SO_SET_REPLACE.
+//
+// Updating this requires running Istio, calling IPT_SO_GET_INFO and
+// IPT_SO_GET_ENTRIES, then stitching the structs up. So be careful when
+// updating!
+//
+//go:embed istio_blob
+var istioBlob []byte
+
 // FakeIDMapper implements IDMapper.
 type FakeIDMapper struct{}
 
@@ -36,50 +45,12 @@ func (*FakeIDMapper) MapToKGID(auth.GID) auth.KGID {
 	return 0
 }
 
-// istioBlob is an iptables NAT ruleset generated by Istio. It is already in
-// the format ready to by passed to IPT_SO_SET_REPLACE.
-//
-// Updating this requires running Istio, calling IPT_SO_GET_INFO and
-// IPT_SO_GET_ENTRIES, then stitching the structs up. So be careful when
-// updating!
-//
-//go:embed istio_blob
-var istioBlob []byte
-
 // TestIstioBlob tests that we support the iptables ruleset generated by Istio,
 // i.e. that we can parse the rules and set them in netstack.
 func TestIstioBlob(t *testing.T) {
 	mapper := FakeIDMapper{}
-	stk := stack.New(stack.Options{})
-	if err := netfilter.SetEntries(&mapper, stk, istioBlob, false); err != nil {
+	stack := stack.New(stack.Options{})
+	if err := SetEntries(&mapper, stack, istioBlob, false); err != nil {
 		t.Fatalf("failed to set Istio rules, try setting enableLogging and running again: %v", err)
 	}
 }
-
-// acceptBlob is an iptables NAT ruleset that instructs iptables to ACCEPT all
-// and modify nothing. This is the Linux default: a newly booted machine just
-// doesn't NAT anything until someone sets the rules.
-//
-// Updating this requires running Istio, calling IPT_SO_GET_INFO and
-// IPT_SO_GET_ENTRIES, then stitching the structs up. So be careful when
-// updating!
-//
-//go:embed accept_blob
-var acceptBlob []byte
-
-// TestAcceptBlob tests that updating the default (all ACCEPT) NAT table with
-// an identical table doesn't actually set the modified flag. In doing so, we
-// can preserve our performance optimization in which iptables is off until
-// non-default rules are set.
-func TestAcceptBlob(t *testing.T) {
-	mapper := FakeIDMapper{}
-	stk := stack.New(stack.Options{
-		DefaultIPTables: netfilter.DefaultLinuxTables,
-	})
-	if err := netfilter.SetEntries(&mapper, stk, acceptBlob, false); err != nil {
-		t.Fatalf("failed to set Istio rules, try setting enableLogging and running again: %v", err)
-	}
-	if stk.IPTables().Modified() {
-		t.Fatalf("ACCEPT rules shouldn't cause iptables modifications, but did")
-	}
-}
diff --git a/pkg/sentry/socket/netfilter/tcp_matcher.go b/pkg/sentry/socket/netfilter/tcp_matcher.go
@@ -49,8 +49,6 @@ func (tcpMarshaler) marshal(mr matcher) []byte {
 		SourcePortEnd:        matcher.sourcePortEnd,
 		DestinationPortStart: matcher.destinationPortStart,
 		DestinationPortEnd:   matcher.destinationPortEnd,
-		FlagMask:             matcher.flagMask,
-		FlagCompare:          matcher.flagCompare,
 	}
 	return marshalEntryMatch(matcherNameTCP, marshal.Marshal(&xttcp))
 }
@@ -67,7 +65,10 @@ func (tcpMarshaler) unmarshal(_ IDMapper, buf []byte, filter stack.IPHeaderFilte
 	matchData.UnmarshalUnsafe(buf)
 	nflog("parseMatchers: parsed XTTCP: %+v", matchData)
 
-	if matchData.Option != 0 || matchData.InverseFlags != 0 {
+	if matchData.Option != 0 ||
+		matchData.FlagMask != 0 ||
+		matchData.FlagCompare != 0 ||
+		matchData.InverseFlags != 0 {
 		return nil, fmt.Errorf("unsupported TCP matcher flags set")
 	}
 
@@ -80,8 +81,6 @@ func (tcpMarshaler) unmarshal(_ IDMapper, buf []byte, filter stack.IPHeaderFilte
 		sourcePortEnd:        matchData.SourcePortEnd,
 		destinationPortStart: matchData.DestinationPortStart,
 		destinationPortEnd:   matchData.DestinationPortEnd,
-		flagMask:             matchData.FlagMask,
-		flagCompare:          matchData.FlagCompare,
 	}, nil
 }
 
@@ -91,8 +90,6 @@ type TCPMatcher struct {
 	sourcePortEnd        uint16
 	destinationPortStart uint16
 	destinationPortEnd   uint16
-	flagMask             uint8
-	flagCompare          uint8
 }
 
 // name implements matcher.name.
@@ -149,10 +146,5 @@ func (tm *TCPMatcher) Match(hook stack.Hook, pkt stack.PacketBufferPtr, _, _ str
 		return false, false
 	}
 
-	// Check the flags.
-	if uint8(tcpHeader.Flags())&tm.flagMask != tm.flagCompare {
-		return false, false
-	}
-
 	return true, false
 }
diff --git a/pkg/tcpip/network/ipv4/ipv4_test.go b/pkg/tcpip/network/ipv4/ipv4_test.go
@@ -3388,7 +3388,7 @@ func TestWriteStats(t *testing.T) {
 				filter := ipt.GetTable(stack.FilterID, false /* ipv6 */)
 				ruleIdx := filter.BuiltinChains[stack.Output]
 				filter.Rules[ruleIdx].Target = &stack.DropTarget{}
-				ipt.ForceReplaceTable(stack.FilterID, filter, false /* ipv6 */)
+				ipt.ReplaceTable(stack.FilterID, filter, false /* ipv6 */)
 			},
 			allowPackets:             math.MaxInt32,
 			expectSent:               0,
@@ -3402,7 +3402,7 @@ func TestWriteStats(t *testing.T) {
 				filter := ipt.GetTable(stack.NATID, false /* ipv6 */)
 				ruleIdx := filter.BuiltinChains[stack.Postrouting]
 				filter.Rules[ruleIdx].Target = &stack.DropTarget{}
-				ipt.ForceReplaceTable(stack.NATID, filter, false /* ipv6 */)
+				ipt.ReplaceTable(stack.NATID, filter, false /* ipv6 */)
 			},
 			allowPackets:             math.MaxInt32,
 			expectSent:               0,
@@ -3422,7 +3422,7 @@ func TestWriteStats(t *testing.T) {
 				filter.Rules[ruleIdx].Matchers = []stack.Matcher{&limitedMatcher{nPackets - 1}}
 				// Make sure the next rule is ACCEPT.
 				filter.Rules[ruleIdx+1].Target = &stack.AcceptTarget{}
-				ipt.ForceReplaceTable(stack.FilterID, filter, false /* ipv6 */)
+				ipt.ReplaceTable(stack.FilterID, filter, false /* ipv6 */)
 			},
 			allowPackets:             math.MaxInt32,
 			expectSent:               nPackets - 1,
@@ -3442,7 +3442,7 @@ func TestWriteStats(t *testing.T) {
 				filter.Rules[ruleIdx].Matchers = []stack.Matcher{&limitedMatcher{nPackets - 1}}
 				// Make sure the next rule is ACCEPT.
 				filter.Rules[ruleIdx+1].Target = &stack.AcceptTarget{}
-				ipt.ForceReplaceTable(stack.NATID, filter, false /* ipv6 */)
+				ipt.ReplaceTable(stack.NATID, filter, false /* ipv6 */)
 			},
 			allowPackets:             math.MaxInt32,
 			expectSent:               nPackets - 1,
@@ -3805,7 +3805,7 @@ func TestCloseLocking(t *testing.T) {
 			stack.Postrouting: 3,
 		},
 	}
-	s.IPTables().ForceReplaceTable(stack.NATID, table, false /* ipv6 */)
+	s.IPTables().ReplaceTable(stack.NATID, table, false /* ipv6 */)
 
 	e := channel.New(0, defaultMTU, "")
 	defer e.Close()

diff --git a/pkg/tcpip/network/ipv6/ipv6_test.go b/pkg/tcpip/network/ipv6/ipv6_test.go
@@ -2519,7 +2519,7 @@ func TestWriteStats(t *testing.T) {
 				filter := ipt.GetTable(stack.FilterID, true /* ipv6 */)
 				ruleIdx := filter.BuiltinChains[stack.Output]
 				filter.Rules[ruleIdx].Target = &stack.DropTarget{}
-				ipt.ForceReplaceTable(stack.FilterID, filter, true /* ipv6 */)
+				ipt.ReplaceTable(stack.FilterID, filter, true /* ipv6 */)
 			},
 			allowPackets:             math.MaxInt32,
 			expectSent:               0,
@@ -2534,7 +2534,7 @@ func TestWriteStats(t *testing.T) {
 				filter := ipt.GetTable(stack.NATID, true /* ipv6 */)
 				ruleIdx := filter.BuiltinChains[stack.Postrouting]
 				filter.Rules[ruleIdx].Target = &stack.DropTarget{}
-				ipt.ForceReplaceTable(stack.NATID, filter, true /* ipv6 */)
+				ipt.ReplaceTable(stack.NATID, filter, true /* ipv6 */)
 			},
 			allowPackets:             math.MaxInt32,
 			expectSent:               0,
@@ -2554,7 +2554,7 @@ func TestWriteStats(t *testing.T) {
 				filter.Rules[ruleIdx].Matchers = []stack.Matcher{&limitedMatcher{nPackets - 1}}
 				// Make sure the next rule is ACCEPT.
 				filter.Rules[ruleIdx+1].Target = &stack.AcceptTarget{}
-				ipt.ForceReplaceTable(stack.FilterID, filter, true /* ipv6 */)
+				ipt.ReplaceTable(stack.FilterID, filter, true /* ipv6 */)
 			},
 			allowPackets:             math.MaxInt32,
 			expectSent:               nPackets - 1,
@@ -2574,7 +2574,7 @@ func TestWriteStats(t *testing.T) {
 				filter.Rules[ruleIdx].Matchers = []stack.Matcher{&limitedMatcher{nPackets - 1}}
 				// Make sure the next rule is ACCEPT.
 				filter.Rules[ruleIdx+1].Target = &stack.AcceptTarget{}
-				ipt.ForceReplaceTable(stack.NATID, filter, true /* ipv6 */)
+				ipt.ReplaceTable(stack.NATID, filter, true /* ipv6 */)
 			},
 			allowPackets:             math.MaxInt32,
 			expectSent:               nPackets - 1,

diff --git a/pkg/tcpip/stack/iptables.go b/pkg/tcpip/stack/iptables.go
@@ -17,7 +17,6 @@ package stack
 import (
 	"fmt"
 	"math/rand"
-	"reflect"
 	"time"
 
 	"gvisor.dev/gvisor/pkg/tcpip"
@@ -49,11 +48,11 @@ func DefaultTables(clock tcpip.Clock, rand *rand.Rand) *IPTables {
 		v4Tables: [NumTables]Table{
 			NATID: {
 				Rules: []Rule{
-					{Filter: EmptyFilter4(), Target: &AcceptTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
-					{Filter: EmptyFilter4(), Target: &AcceptTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
-					{Filter: EmptyFilter4(), Target: &AcceptTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
-					{Filter: EmptyFilter4(), Target: &AcceptTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
-					{Filter: EmptyFilter4(), Target: &ErrorTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
+					{Target: &AcceptTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
+					{Target: &AcceptTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
+					{Target: &AcceptTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
+					{Target: &AcceptTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
+					{Target: &ErrorTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
 				},
 				BuiltinChains: [NumHooks]int{
 					Prerouting:  0,
@@ -72,9 +71,9 @@ func DefaultTables(clock tcpip.Clock, rand *rand.Rand) *IPTables {
 			},
 			MangleID: {
 				Rules: []Rule{
-					{Filter: EmptyFilter4(), Target: &AcceptTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
-					{Filter: EmptyFilter4(), Target: &AcceptTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
-					{Filter: EmptyFilter4(), Target: &ErrorTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
+					{Target: &AcceptTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
+					{Target: &AcceptTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
+					{Target: &ErrorTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
 				},
 				BuiltinChains: [NumHooks]int{
 					Prerouting: 0,
@@ -90,10 +89,10 @@ func DefaultTables(clock tcpip.Clock, rand *rand.Rand) *IPTables {
 			},
 			FilterID: {
 				Rules: []Rule{
-					{Filter: EmptyFilter4(), Target: &AcceptTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
-					{Filter: EmptyFilter4(), Target: &AcceptTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
-					{Filter: EmptyFilter4(), Target: &AcceptTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
-					{Filter: EmptyFilter4(), Target: &ErrorTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
+					{Target: &AcceptTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
+					{Target: &AcceptTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
+					{Target: &AcceptTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
+					{Target: &ErrorTarget{NetworkProtocol: header.IPv4ProtocolNumber}},
 				},
 				BuiltinChains: [NumHooks]int{
 					Prerouting:  HookUnset,
@@ -114,11 +113,11 @@ func DefaultTables(clock tcpip.Clock, rand *rand.Rand) *IPTables {
 		v6Tables: [NumTables]Table{
 			NATID: {
 				Rules: []Rule{
-					{Filter: EmptyFilter6(), Target: &AcceptTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
-					{Filter: EmptyFilter6(), Target: &AcceptTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
-					{Filter: EmptyFilter6(), Target: &AcceptTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
-					{Filter: EmptyFilter6(), Target: &AcceptTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
-					{Filter: EmptyFilter6(), Target: &ErrorTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
+					{Target: &AcceptTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
+					{Target: &AcceptTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
+					{Target: &AcceptTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
+					{Target: &AcceptTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
+					{Target: &ErrorTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
 				},
 				BuiltinChains: [NumHooks]int{
 					Prerouting:  0,
@@ -137,9 +136,9 @@ func DefaultTables(clock tcpip.Clock, rand *rand.Rand) *IPTables {
 			},
 			MangleID: {
 				Rules: []Rule{
-					{Filter: EmptyFilter6(), Target: &AcceptTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
-					{Filter: EmptyFilter6(), Target: &AcceptTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
-					{Filter: EmptyFilter6(), Target: &ErrorTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
+					{Target: &AcceptTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
+					{Target: &AcceptTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
+					{Target: &ErrorTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
 				},
 				BuiltinChains: [NumHooks]int{
 					Prerouting: 0,
@@ -155,10 +154,10 @@ func DefaultTables(clock tcpip.Clock, rand *rand.Rand) *IPTables {
 			},
 			FilterID: {
 				Rules: []Rule{
-					{Filter: EmptyFilter6(), Target: &AcceptTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
-					{Filter: EmptyFilter6(), Target: &AcceptTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
-					{Filter: EmptyFilter6(), Target: &AcceptTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
-					{Filter: EmptyFilter6(), Target: &ErrorTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
+					{Target: &AcceptTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
+					{Target: &AcceptTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
+					{Target: &AcceptTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
+					{Target: &ErrorTarget{NetworkProtocol: header.IPv6ProtocolNumber}},
 				},
 				BuiltinChains: [NumHooks]int{
 					Prerouting:  HookUnset,
@@ -233,29 +232,11 @@ func (it *IPTables) getTableRLocked(id TableID, ipv6 bool) Table {
 // ReplaceTable replaces or inserts table by name. It panics when an invalid id
 // is provided.
 func (it *IPTables) ReplaceTable(id TableID, table Table, ipv6 bool) {
-	it.replaceTable(id, table, ipv6, false /* force */)
-}
-
-// ForceReplaceTable replaces or inserts table by name. It panics when an invalid id
-// is provided. It enables iptables even when the inserted table is all
-// conditionless ACCEPT, skipping our optimization that disables iptables until
-// they're modified.
-func (it *IPTables) ForceReplaceTable(id TableID, table Table, ipv6 bool) {
-	it.replaceTable(id, table, ipv6, true /* force */)
-}
-
-func (it *IPTables) replaceTable(id TableID, table Table, ipv6, force bool) {
 	it.mu.Lock()
 	defer it.mu.Unlock()
-
 	// If iptables is being enabled, initialize the conntrack table and
 	// reaper.
 	if !it.modified {
-		// Don't do anything if the table is identical.
-		if ((ipv6 && reflect.DeepEqual(table, it.v6Tables[id])) || (!ipv6 && reflect.DeepEqual(table, it.v4Tables[id]))) && !force {
-			return
-		}
-
 		it.connections.init()
 		it.startReaper(reaperDelay)
 	}

diff --git a/pkg/tcpip/stack/iptables_test.go b/pkg/tcpip/stack/iptables_test.go
@@ -300,7 +300,7 @@ func TestNATAlwaysPerformed(t *testing.T) {
 			iptables := DefaultTables(clock, rand.New(rand.NewSource(0 /* seed */)))
 
 			// Just to make sure the iptables is not short circuited.
-			iptables.ForceReplaceTable(NATID, iptables.GetTable(NATID, ipv6), ipv6)
+			iptables.ReplaceTable(NATID, iptables.GetTable(NATID, ipv6), ipv6)
 
 			pkt := v6PacketBuffer()