diff options
author | Nick Brown <nickbrow@google.com> | 2021-10-27 10:03:11 -0700 |
---|---|---|
committer | gVisor bot <gvisor-bot@google.com> | 2021-10-27 10:06:55 -0700 |
commit | 22a6a37079c69129d10abfbdd6fdfdf7a9d4a68d (patch) | |
tree | 6c19c8844d062db058f9fe02840ed4c69387314d | |
parent | 7b8f19dc76a9fecbf4d2e5f43a47c6d47d53e100 (diff) |
Record counts of packets with unknown L3/L4 numbers
Previously, we recorded a single aggregated count. These per-protocol counts
can help us debug field issues when frames are dropped for this reason.
PiperOrigin-RevId: 405913911
-rw-r--r-- | pkg/sentry/socket/netstack/netstack.go | 4 | ||||
-rw-r--r-- | pkg/tcpip/network/arp/stats_test.go | 5 | ||||
-rw-r--r-- | pkg/tcpip/network/ipv4/stats_test.go | 15 | ||||
-rw-r--r-- | pkg/tcpip/network/ipv6/ipv6_test.go | 10 | ||||
-rw-r--r-- | pkg/tcpip/stack/nic.go | 4 | ||||
-rw-r--r-- | pkg/tcpip/stack/nic_stats.go | 24 | ||||
-rw-r--r-- | pkg/tcpip/stack/nic_test.go | 44 | ||||
-rw-r--r-- | pkg/tcpip/tcpip.go | 92 | ||||
-rw-r--r-- | pkg/tcpip/testutil/testutil.go | 85 |
9 files changed, 243 insertions, 40 deletions
diff --git a/pkg/sentry/socket/netstack/netstack.go b/pkg/sentry/socket/netstack/netstack.go index 030c6c8e4..2a1c2f246 100644 --- a/pkg/sentry/socket/netstack/netstack.go +++ b/pkg/sentry/socket/netstack/netstack.go @@ -81,9 +81,7 @@ func mustCreateGauge(name, description string) *tcpip.StatCounter { var Metrics = tcpip.Stats{ DroppedPackets: mustCreateMetric("/netstack/dropped_packets", "Number of packets dropped at the transport layer."), NICs: tcpip.NICStats{ - UnknownL3ProtocolRcvdPackets: mustCreateMetric("/netstack/nic/unknown_l3_protocol_received_packets", "Number of packets received that were for an unknown or unsupported L3 protocol."), - UnknownL4ProtocolRcvdPackets: mustCreateMetric("/netstack/nic/unknown_l4_protocol_received_packets", "Number of packets received that were for an unknown or unsupported L4 protocol."), - MalformedL4RcvdPackets: mustCreateMetric("/netstack/nic/malformed_l4_received_packets", "Number of packets received that failed L4 header parsing."), + MalformedL4RcvdPackets: mustCreateMetric("/netstack/nic/malformed_l4_received_packets", "Number of packets received that failed L4 header parsing."), Tx: tcpip.NICPacketStats{ Packets: mustCreateMetric("/netstack/nic/tx/packets", "Number of packets transmitted."), Bytes: mustCreateMetric("/netstack/nic/tx/bytes", "Number of bytes transmitted."), diff --git a/pkg/tcpip/network/arp/stats_test.go b/pkg/tcpip/network/arp/stats_test.go index 0df39ae81..c7d97afec 100644 --- a/pkg/tcpip/network/arp/stats_test.go +++ b/pkg/tcpip/network/arp/stats_test.go @@ -45,7 +45,10 @@ func TestMultiCounterStatsInitialization(t *testing.T) { // expected to be bound by a MultiCounterStat. refStack := s.Stats() refEP := ep.stats.localStats - if err := testutil.ValidateMultiCounterStats(reflect.ValueOf(&ep.stats.arp).Elem(), []reflect.Value{reflect.ValueOf(&refEP.ARP).Elem(), reflect.ValueOf(&refStack.ARP).Elem()}); err != nil { + if err := testutil.ValidateMultiCounterStats(reflect.ValueOf(&ep.stats.arp).Elem(), []reflect.Value{reflect.ValueOf(&refEP.ARP).Elem(), reflect.ValueOf(&refStack.ARP).Elem()}, testutil.ValidateMultiCounterStatsOptions{ + ExpectMultiCounterStat: true, + ExpectMultiIntegralStatCounterMap: false, + }); err != nil { t.Error(err) } } diff --git a/pkg/tcpip/network/ipv4/stats_test.go b/pkg/tcpip/network/ipv4/stats_test.go index d1f9e3cf5..15a056d4b 100644 --- a/pkg/tcpip/network/ipv4/stats_test.go +++ b/pkg/tcpip/network/ipv4/stats_test.go @@ -87,13 +87,22 @@ func TestMultiCounterStatsInitialization(t *testing.T) { // expected to be bound by a MultiCounterStat. refStack := s.Stats() refEP := ep.stats.localStats - if err := testutil.ValidateMultiCounterStats(reflect.ValueOf(&ep.stats.ip).Elem(), []reflect.Value{reflect.ValueOf(&refEP.IP).Elem(), reflect.ValueOf(&refStack.IP).Elem()}); err != nil { + if err := testutil.ValidateMultiCounterStats(reflect.ValueOf(&ep.stats.ip).Elem(), []reflect.Value{reflect.ValueOf(&refEP.IP).Elem(), reflect.ValueOf(&refStack.IP).Elem()}, testutil.ValidateMultiCounterStatsOptions{ + ExpectMultiCounterStat: true, + ExpectMultiIntegralStatCounterMap: false, + }); err != nil { t.Error(err) } - if err := testutil.ValidateMultiCounterStats(reflect.ValueOf(&ep.stats.icmp).Elem(), []reflect.Value{reflect.ValueOf(&refEP.ICMP).Elem(), reflect.ValueOf(&refStack.ICMP.V4).Elem()}); err != nil { + if err := testutil.ValidateMultiCounterStats(reflect.ValueOf(&ep.stats.icmp).Elem(), []reflect.Value{reflect.ValueOf(&refEP.ICMP).Elem(), reflect.ValueOf(&refStack.ICMP.V4).Elem()}, testutil.ValidateMultiCounterStatsOptions{ + ExpectMultiCounterStat: true, + ExpectMultiIntegralStatCounterMap: false, + }); err != nil { t.Error(err) } - if err := testutil.ValidateMultiCounterStats(reflect.ValueOf(&ep.stats.igmp).Elem(), []reflect.Value{reflect.ValueOf(&refEP.IGMP).Elem(), reflect.ValueOf(&refStack.IGMP).Elem()}); err != nil { + if err := testutil.ValidateMultiCounterStats(reflect.ValueOf(&ep.stats.igmp).Elem(), []reflect.Value{reflect.ValueOf(&refEP.IGMP).Elem(), reflect.ValueOf(&refStack.IGMP).Elem()}, testutil.ValidateMultiCounterStatsOptions{ + ExpectMultiCounterStat: true, + ExpectMultiIntegralStatCounterMap: false, + }); err != nil { t.Error(err) } } diff --git a/pkg/tcpip/network/ipv6/ipv6_test.go b/pkg/tcpip/network/ipv6/ipv6_test.go index e5286081e..9e08b5318 100644 --- a/pkg/tcpip/network/ipv6/ipv6_test.go +++ b/pkg/tcpip/network/ipv6/ipv6_test.go @@ -3515,10 +3515,16 @@ func TestMultiCounterStatsInitialization(t *testing.T) { // supposed to be bound. refStack := s.Stats() refEP := ep.stats.localStats - if err := testutil.ValidateMultiCounterStats(reflect.ValueOf(&ep.stats.ip).Elem(), []reflect.Value{reflect.ValueOf(&refStack.IP).Elem(), reflect.ValueOf(&refEP.IP).Elem()}); err != nil { + if err := testutil.ValidateMultiCounterStats(reflect.ValueOf(&ep.stats.ip).Elem(), []reflect.Value{reflect.ValueOf(&refStack.IP).Elem(), reflect.ValueOf(&refEP.IP).Elem()}, testutil.ValidateMultiCounterStatsOptions{ + ExpectMultiCounterStat: true, + ExpectMultiIntegralStatCounterMap: false, + }); err != nil { t.Error(err) } - if err := testutil.ValidateMultiCounterStats(reflect.ValueOf(&ep.stats.icmp).Elem(), []reflect.Value{reflect.ValueOf(&refStack.ICMP.V6).Elem(), reflect.ValueOf(&refEP.ICMP).Elem()}); err != nil { + if err := testutil.ValidateMultiCounterStats(reflect.ValueOf(&ep.stats.icmp).Elem(), []reflect.Value{reflect.ValueOf(&refStack.ICMP.V6).Elem(), reflect.ValueOf(&refEP.ICMP).Elem()}, testutil.ValidateMultiCounterStatsOptions{ + ExpectMultiCounterStat: true, + ExpectMultiIntegralStatCounterMap: false, + }); err != nil { t.Error(err) } } diff --git a/pkg/tcpip/stack/nic.go b/pkg/tcpip/stack/nic.go index e251e3b24..b9b5c35c8 100644 --- a/pkg/tcpip/stack/nic.go +++ b/pkg/tcpip/stack/nic.go @@ -727,7 +727,7 @@ func (n *nic) DeliverNetworkPacket(remote, local tcpip.LinkAddress, protocol tcp networkEndpoint, ok := n.networkEndpoints[protocol] if !ok { - n.stats.unknownL3ProtocolRcvdPackets.Increment() + n.stats.unknownL3ProtocolRcvdPacketCounts.Increment(uint64(protocol)) return } @@ -827,7 +827,7 @@ func (n *nic) deliverOutboundPacket(remote tcpip.LinkAddress, pkt *PacketBuffer) func (n *nic) DeliverTransportPacket(protocol tcpip.TransportProtocolNumber, pkt *PacketBuffer) TransportPacketDisposition { state, ok := n.stack.transportProtocols[protocol] if !ok { - n.stats.unknownL4ProtocolRcvdPackets.Increment() + n.stats.unknownL4ProtocolRcvdPacketCounts.Increment(uint64(protocol)) return TransportPacketProtocolUnreachable } diff --git a/pkg/tcpip/stack/nic_stats.go b/pkg/tcpip/stack/nic_stats.go index 1773d5e8d..89aa95131 100644 --- a/pkg/tcpip/stack/nic_stats.go +++ b/pkg/tcpip/stack/nic_stats.go @@ -35,7 +35,7 @@ func (m *multiCounterNICPacketStats) init(a, b *tcpip.NICPacketStats) { m.bytes.Init(a.Bytes, b.Bytes) } -// LINT.ThenChange(../../tcpip.go:NICPacketStats) +// LINT.ThenChange(../tcpip.go:NICPacketStats) // LINT.IfChange(multiCounterNICNeighborStats) @@ -47,23 +47,23 @@ func (m *multiCounterNICNeighborStats) init(a, b *tcpip.NICNeighborStats) { m.unreachableEntryLookups.Init(a.UnreachableEntryLookups, b.UnreachableEntryLookups) } -// LINT.ThenChange(../../tcpip.go:NICNeighborStats) +// LINT.ThenChange(../tcpip.go:NICNeighborStats) // LINT.IfChange(multiCounterNICStats) type multiCounterNICStats struct { - unknownL3ProtocolRcvdPackets tcpip.MultiCounterStat - unknownL4ProtocolRcvdPackets tcpip.MultiCounterStat - malformedL4RcvdPackets tcpip.MultiCounterStat - tx multiCounterNICPacketStats - rx multiCounterNICPacketStats - disabledRx multiCounterNICPacketStats - neighbor multiCounterNICNeighborStats + unknownL3ProtocolRcvdPacketCounts tcpip.MultiIntegralStatCounterMap + unknownL4ProtocolRcvdPacketCounts tcpip.MultiIntegralStatCounterMap + malformedL4RcvdPackets tcpip.MultiCounterStat + tx multiCounterNICPacketStats + rx multiCounterNICPacketStats + disabledRx multiCounterNICPacketStats + neighbor multiCounterNICNeighborStats } func (m *multiCounterNICStats) init(a, b *tcpip.NICStats) { - m.unknownL3ProtocolRcvdPackets.Init(a.UnknownL3ProtocolRcvdPackets, b.UnknownL3ProtocolRcvdPackets) - m.unknownL4ProtocolRcvdPackets.Init(a.UnknownL4ProtocolRcvdPackets, b.UnknownL4ProtocolRcvdPackets) + m.unknownL3ProtocolRcvdPacketCounts.Init(a.UnknownL3ProtocolRcvdPacketCounts, b.UnknownL3ProtocolRcvdPacketCounts) + m.unknownL4ProtocolRcvdPacketCounts.Init(a.UnknownL4ProtocolRcvdPacketCounts, b.UnknownL4ProtocolRcvdPacketCounts) m.malformedL4RcvdPackets.Init(a.MalformedL4RcvdPackets, b.MalformedL4RcvdPackets) m.tx.init(&a.Tx, &b.Tx) m.rx.init(&a.Rx, &b.Rx) @@ -71,4 +71,4 @@ func (m *multiCounterNICStats) init(a, b *tcpip.NICStats) { m.neighbor.init(&a.Neighbor, &b.Neighbor) } -// LINT.ThenChange(../../tcpip.go:NICStats) +// LINT.ThenChange(../tcpip.go:NICStats) diff --git a/pkg/tcpip/stack/nic_test.go b/pkg/tcpip/stack/nic_test.go index c8ad93f29..88ca9b076 100644 --- a/pkg/tcpip/stack/nic_test.go +++ b/pkg/tcpip/stack/nic_test.go @@ -206,6 +206,45 @@ func TestDisabledRxStatsWhenNICDisabled(t *testing.T) { } } +func TestPacketWithUnknownNetworkProtocolNumber(t *testing.T) { + nic := nic{ + stats: makeNICStats(tcpip.NICStats{}.FillIn()), + enabled: 1, + } + // IPv4 isn't recognized since we haven't initialized the NIC with an IPv4 + // endpoint. + nic.DeliverNetworkPacket("", "", header.IPv4ProtocolNumber, NewPacketBuffer(PacketBufferOptions{ + Data: buffer.View([]byte{1, 2, 3, 4}).ToVectorisedView(), + })) + var count uint64 + if got, ok := nic.stats.local.UnknownL3ProtocolRcvdPacketCounts.Get(uint64(header.IPv4ProtocolNumber)); ok { + count = got.Value() + } + if count != 1 { + t.Errorf("got UnknownL3ProtocolRcvdPacketCounts[header.IPv4ProtocolNumber] = %d, want = 1", count) + } +} + +func TestPacketWithUnknownTransportProtocolNumber(t *testing.T) { + nic := nic{ + stack: &Stack{}, + stats: makeNICStats(tcpip.NICStats{}.FillIn()), + enabled: 1, + } + // UDP isn't recognized since we haven't initialized the NIC with a UDP + // protocol. + nic.DeliverTransportPacket(header.UDPProtocolNumber, NewPacketBuffer(PacketBufferOptions{ + Data: buffer.View([]byte{1, 2, 3, 4}).ToVectorisedView(), + })) + var count uint64 + if got, ok := nic.stats.local.UnknownL4ProtocolRcvdPacketCounts.Get(uint64(header.UDPProtocolNumber)); ok { + count = got.Value() + } + if count != 1 { + t.Errorf("got UnknownL4ProtocolRcvdPacketCounts[header.UDPProtocolNumber] = %d, want = 1", count) + } +} + func TestMultiCounterStatsInitialization(t *testing.T) { global := tcpip.NICStats{}.FillIn() nic := nic{ @@ -213,7 +252,10 @@ func TestMultiCounterStatsInitialization(t *testing.T) { } multi := nic.stats.multiCounterNICStats local := nic.stats.local - if err := testutil.ValidateMultiCounterStats(reflect.ValueOf(&multi).Elem(), []reflect.Value{reflect.ValueOf(&local).Elem(), reflect.ValueOf(&global).Elem()}); err != nil { + if err := testutil.ValidateMultiCounterStats(reflect.ValueOf(&multi).Elem(), []reflect.Value{reflect.ValueOf(&local).Elem(), reflect.ValueOf(&global).Elem()}, testutil.ValidateMultiCounterStatsOptions{ + ExpectMultiCounterStat: true, + ExpectMultiIntegralStatCounterMap: true, + }); err != nil { t.Error(err) } } diff --git a/pkg/tcpip/tcpip.go b/pkg/tcpip/tcpip.go index 460a6afaf..d20dd495c 100644 --- a/pkg/tcpip/tcpip.go +++ b/pkg/tcpip/tcpip.go @@ -1301,7 +1301,8 @@ func (s *StatCounter) String() string { // A MultiCounterStat keeps track of two counters at once. type MultiCounterStat struct { - a, b *StatCounter + a *StatCounter + b *StatCounter } // Init sets both internal counters to point to a and b. @@ -1923,17 +1924,89 @@ type NICPacketStats struct { // LINT.ThenChange(stack/nic_stats.go:multiCounterNICPacketStats) } +// IntegralStatCounterMap holds a map associating integral keys with +// StatCounters. +type IntegralStatCounterMap struct { + mu sync.RWMutex + // +checklocks:mu + counterMap map[uint64]*StatCounter +} + +// Keys returns all keys present in the map. +func (m *IntegralStatCounterMap) Keys() []uint64 { + m.mu.RLock() + defer m.mu.RUnlock() + var keys []uint64 + for k := range m.counterMap { + keys = append(keys, k) + } + return keys +} + +// Get returns the counter mapped by the provided key. +func (m *IntegralStatCounterMap) Get(key uint64) (*StatCounter, bool) { + m.mu.RLock() + defer m.mu.RUnlock() + counter, ok := m.counterMap[key] + return counter, ok +} + +// Init initializes the map. +func (m *IntegralStatCounterMap) Init() { + m.mu.Lock() + defer m.mu.Unlock() + m.counterMap = make(map[uint64]*StatCounter) +} + +// Increment increments the counter associated with the provided key. +func (m *IntegralStatCounterMap) Increment(key uint64) { + m.mu.RLock() + counter, ok := m.counterMap[key] + m.mu.RUnlock() + + if !ok { + m.mu.Lock() + counter, ok = m.counterMap[key] + if !ok { + counter = new(StatCounter) + m.counterMap[key] = counter + } + m.mu.Unlock() + } + counter.Increment() +} + +// A MultiIntegralStatCounterMap keeps track of two integral counter maps at +// once. +type MultiIntegralStatCounterMap struct { + a *IntegralStatCounterMap + b *IntegralStatCounterMap +} + +// Init sets the internal integral counter maps to point to a and b. +func (m *MultiIntegralStatCounterMap) Init(a, b *IntegralStatCounterMap) { + m.a = a + m.b = b +} + +// Increment increments the counter in each map corresponding to the +// provided key. +func (m *MultiIntegralStatCounterMap) Increment(key uint64) { + m.a.Increment(key) + m.b.Increment(key) +} + // NICStats holds NIC statistics. type NICStats struct { // LINT.IfChange(NICStats) - // UnknownL3ProtocolRcvdPackets is the number of packets received that were - // for an unknown or unsupported network protocol. - UnknownL3ProtocolRcvdPackets *StatCounter + // UnknownL3ProtocolRcvdPacketCounts records the number of packets recieved + // for each unknown or unsupported netowrk protocol number. + UnknownL3ProtocolRcvdPacketCounts *IntegralStatCounterMap - // UnknownL4ProtocolRcvdPackets is the number of packets received that were - // for an unknown or unsupported transport protocol. - UnknownL4ProtocolRcvdPackets *StatCounter + // UnknownL4ProtocolRcvdPacketCounts records the number of packets recieved + // for each unknown or unsupported transport protocol number. + UnknownL4ProtocolRcvdPacketCounts *IntegralStatCounterMap // MalformedL4RcvdPackets is the number of packets received by a NIC that // could not be delivered to a transport endpoint because the L4 header could @@ -2103,6 +2176,11 @@ func InitStatCounters(v reflect.Value) { if *s == nil { *s = new(StatCounter) } + } else if s, ok := v.Addr().Interface().(**IntegralStatCounterMap); ok { + if *s == nil { + *s = new(IntegralStatCounterMap) + (*s).Init() + } } else { InitStatCounters(v) } diff --git a/pkg/tcpip/testutil/testutil.go b/pkg/tcpip/testutil/testutil.go index 94b580a70..903fe250d 100644 --- a/pkg/tcpip/testutil/testutil.go +++ b/pkg/tcpip/testutil/testutil.go @@ -77,12 +77,43 @@ func validateField(ref reflect.Value, refName string, m tcpip.MultiCounterStat, return nil } -// ValidateMultiCounterStats verifies that every counter stored in multi is -// correctly tracking its counterpart in the given counters. -func ValidateMultiCounterStats(multi reflect.Value, counters []reflect.Value) error { +func validateIntegralMapField(ref reflect.Value, refName string, m tcpip.MultiIntegralStatCounterMap, multiName string) error { + // The field names are expected to match (case insensitive). + if !strings.EqualFold(refName, multiName) { + return fmt.Errorf("wrong field name: got = %s, want = %s", multiName, refName) + } + s, ok := ref.Addr().Interface().(**tcpip.IntegralStatCounterMap) + if !ok { + return fmt.Errorf("field is not an IntegralStatCounterMap") + } + + const key = 42 + + getValue := func() uint64 { + counter, ok := (*s).Get(key) + if !ok { + return 0 + } + return counter.Value() + } + + before := getValue() + + m.Increment(key) + + after := getValue() + + if after != before+1 { + return fmt.Errorf("updates to the '%s MultiCounterStat' counters are not reflected in the '%s CounterStat'", multiName, refName) + } + + return nil +} + +func validateMultiCounterStats(multi reflect.Value, counters []reflect.Value) (foundMultiCounterStat, foundMultiIntegralStatCounterMap bool, err error) { for _, c := range counters { if err := checkFieldCounts(c, multi); err != nil { - return err + return false, false, err } } @@ -90,23 +121,59 @@ func ValidateMultiCounterStats(multi reflect.Value, counters []reflect.Value) er multiName := multi.Type().Field(i).Name multiUnsafe := unsafeExposeUnexportedFields(multi.Field(i)) - if m, ok := multiUnsafe.Addr().Interface().(*tcpip.MultiCounterStat); ok { + switch m := multiUnsafe.Addr().Interface().(type) { + case *tcpip.MultiCounterStat: + foundMultiCounterStat = true for _, c := range counters { if err := validateField(unsafeExposeUnexportedFields(c.Field(i)), c.Type().Field(i).Name, *m, multiName); err != nil { - return err + return false, false, err } } - } else { + case *tcpip.MultiIntegralStatCounterMap: + foundMultiIntegralStatCounterMap = true + for _, c := range counters { + if err := validateIntegralMapField(unsafeExposeUnexportedFields(c.Field(i)), c.Type().Field(i).Name, *m, multiName); err != nil { + return false, false, err + } + } + default: var countersNextField []reflect.Value for _, c := range counters { countersNextField = append(countersNextField, c.Field(i)) } - if err := ValidateMultiCounterStats(multi.Field(i), countersNextField); err != nil { - return err + innerFoundMultiCounterStat, innerFoundMultiIntegralStatCounterMap, err := validateMultiCounterStats(multi.Field(i), countersNextField) + if err != nil { + return false, false, err } + foundMultiCounterStat = foundMultiCounterStat || innerFoundMultiCounterStat + foundMultiIntegralStatCounterMap = foundMultiIntegralStatCounterMap || innerFoundMultiIntegralStatCounterMap } } + return foundMultiCounterStat, foundMultiIntegralStatCounterMap, nil +} + +// ValidateMultiCounterStatsOptions holds options used when validating multi +// counter stat structs. +type ValidateMultiCounterStatsOptions struct { + ExpectMultiCounterStat bool + ExpectMultiIntegralStatCounterMap bool +} + +// ValidateMultiCounterStats verifies that every counter stored in multi is +// correctly tracking its counterpart in the given counters. +func ValidateMultiCounterStats(multi reflect.Value, counters []reflect.Value, options ValidateMultiCounterStatsOptions) error { + foundMultiCounterStat, foundMultiIntegralStatCounterMap, err := validateMultiCounterStats(multi, counters) + if err != nil { + return err + } + if foundMultiCounterStat != options.ExpectMultiCounterStat { + return fmt.Errorf("got %T presence: %t, want: %t", (*tcpip.MultiCounterStat)(nil), foundMultiCounterStat, options.ExpectMultiCounterStat) + } + if foundMultiIntegralStatCounterMap != options.ExpectMultiIntegralStatCounterMap { + return fmt.Errorf("got %T presence: %t, want: %t", (*tcpip.MultiIntegralStatCounterMap)(nil), foundMultiIntegralStatCounterMap, options.ExpectMultiIntegralStatCounterMap) + } + return nil } |