diff --git a/pkg/tcpip/network/ipv4/icmp.go b/pkg/tcpip/network/ipv4/icmp.go index b6da8bd40..8e96ca803 100644 --- a/pkg/tcpip/network/ipv4/icmp.go +++ b/pkg/tcpip/network/ipv4/icmp.go @@ -232,8 +232,7 @@ func (e *endpoint) checkLocalAddress(addr tcpip.Address) bool { return true } - if addressEndpoint := e.AcquireAssignedAddress(addr, false, stack.NeverPrimaryEndpoint); addressEndpoint != nil { - addressEndpoint.DecRef() + if addressEndpoint := e.AcquireAssignedAddress(addr, false, stack.NeverPrimaryEndpoint, true /* readOnly */); addressEndpoint != nil { return true } return false diff --git a/pkg/tcpip/network/ipv4/ipv4.go b/pkg/tcpip/network/ipv4/ipv4.go index d8fff5368..d7eab68c6 100644 --- a/pkg/tcpip/network/ipv4/ipv4.go +++ b/pkg/tcpip/network/ipv4/ipv4.go @@ -193,8 +193,7 @@ func (p *protocol) findEndpointWithAddress(addr tcpip.Address) *endpoint { defer p.mu.RUnlock() for _, e := range p.eps { - if addressEndpoint := e.AcquireAssignedAddress(addr, false /* allowTemp */, stack.NeverPrimaryEndpoint); addressEndpoint != nil { - addressEndpoint.DecRef() + if addressEndpoint := e.AcquireAssignedAddress(addr, false /* allowTemp */, stack.NeverPrimaryEndpoint, true /* readOnly */); addressEndpoint != nil { return e } } @@ -846,10 +845,8 @@ func (e *endpoint) HandlePacket(pkt *stack.PacketBuffer) { } if e.protocol.stack.HandleLocal() { - addressEndpoint := e.AcquireAssignedAddress(header.IPv4(pkt.NetworkHeader().Slice()).SourceAddress(), e.nic.Promiscuous(), stack.CanBePrimaryEndpoint) + addressEndpoint := e.AcquireAssignedAddress(header.IPv4(pkt.NetworkHeader().Slice()).SourceAddress(), e.nic.Promiscuous(), stack.CanBePrimaryEndpoint, true /* readOnly */) if addressEndpoint != nil { - addressEndpoint.DecRef() - // The source address is one of our own, so we never should have gotten // a packet like this unless HandleLocal is false or our NIC is the // loopback interface. @@ -1118,9 +1115,8 @@ func (e *endpoint) handleValidatedPacket(h header.IPv4, pkt *stack.PacketBuffer, return } // Make sure the source address is not a subnet-local broadcast address. - if addressEndpoint := e.AcquireAssignedAddress(srcAddr, false /* createTemp */, stack.NeverPrimaryEndpoint); addressEndpoint != nil { + if addressEndpoint := e.AcquireAssignedAddress(srcAddr, false /* createTemp */, stack.NeverPrimaryEndpoint, true /* readOnly */); addressEndpoint != nil { subnet := addressEndpoint.Subnet() - addressEndpoint.DecRef() if subnet.IsBroadcast(srcAddr) { stats.ip.InvalidSourceAddressesReceived.Increment() return @@ -1157,9 +1153,8 @@ func (e *endpoint) handleValidatedPacket(h header.IPv4, pkt *stack.PacketBuffer, // // If the packet is destined for this device, then it should be delivered // locally. Otherwise, if forwarding is enabled, it should be forwarded. - if addressEndpoint := e.AcquireAssignedAddress(dstAddr, e.nic.Promiscuous(), stack.CanBePrimaryEndpoint); addressEndpoint != nil { + if addressEndpoint := e.AcquireAssignedAddress(dstAddr, e.nic.Promiscuous(), stack.CanBePrimaryEndpoint, true /* readOnly */); addressEndpoint != nil { subnet := addressEndpoint.AddressWithPrefix().Subnet() - addressEndpoint.DecRef() pkt.NetworkPacketInfo.LocalAddressBroadcast = subnet.IsBroadcast(dstAddr) || dstAddr == header.IPv4Broadcast e.deliverPacketLocally(h, pkt, inNICName) } else if e.Forwarding() { @@ -1409,7 +1404,7 @@ func (e *endpoint) MainAddress() tcpip.AddressWithPrefix { } // AcquireAssignedAddress implements stack.AddressableEndpoint. -func (e *endpoint) AcquireAssignedAddress(localAddr tcpip.Address, allowTemp bool, tempPEB stack.PrimaryEndpointBehavior) stack.AddressEndpoint { +func (e *endpoint) AcquireAssignedAddress(localAddr tcpip.Address, allowTemp bool, tempPEB stack.PrimaryEndpointBehavior, readOnly bool) stack.AddressEndpoint { e.mu.RLock() defer e.mu.RUnlock() @@ -1419,7 +1414,7 @@ func (e *endpoint) AcquireAssignedAddress(localAddr tcpip.Address, allowTemp boo // IPv4 has a notion of a subnet broadcast address and considers the // loopback interface bound to an address's whole subnet (on linux). return subnet.IsBroadcast(localAddr) || (loopback && subnet.Contains(localAddr)) - }, allowTemp, tempPEB) + }, allowTemp, tempPEB, readOnly) } // AcquireOutgoingPrimaryAddress implements stack.AddressableEndpoint. @@ -1758,9 +1753,8 @@ func (p *protocol) isSubnetLocalBroadcastAddress(addr tcpip.Address) bool { defer p.mu.RUnlock() for _, e := range p.eps { - if addressEndpoint := e.AcquireAssignedAddress(addr, false /* createTemp */, stack.NeverPrimaryEndpoint); addressEndpoint != nil { + if addressEndpoint := e.AcquireAssignedAddress(addr, false /* createTemp */, stack.NeverPrimaryEndpoint, true /* readOnly */); addressEndpoint != nil { subnet := addressEndpoint.Subnet() - addressEndpoint.DecRef() if subnet.IsBroadcast(addr) { return true } diff --git a/pkg/tcpip/network/ipv6/icmp.go b/pkg/tcpip/network/ipv6/icmp.go index 4028f91df..eb85f4505 100644 --- a/pkg/tcpip/network/ipv6/icmp.go +++ b/pkg/tcpip/network/ipv6/icmp.go @@ -153,8 +153,7 @@ func (e *endpoint) checkLocalAddress(addr tcpip.Address) bool { return true } - if addressEndpoint := e.AcquireAssignedAddress(addr, false, stack.NeverPrimaryEndpoint); addressEndpoint != nil { - addressEndpoint.DecRef() + if addressEndpoint := e.AcquireAssignedAddress(addr, false, stack.NeverPrimaryEndpoint, true /* readOnly */); addressEndpoint != nil { return true } return false diff --git a/pkg/tcpip/network/ipv6/ipv6.go b/pkg/tcpip/network/ipv6/ipv6.go index b0da8b4b5..a846ca430 100644 --- a/pkg/tcpip/network/ipv6/ipv6.go +++ b/pkg/tcpip/network/ipv6/ipv6.go @@ -1105,10 +1105,8 @@ func (e *endpoint) HandlePacket(pkt *stack.PacketBuffer) { } if e.protocol.stack.HandleLocal() { - addressEndpoint := e.AcquireAssignedAddress(header.IPv6(pkt.NetworkHeader().Slice()).SourceAddress(), e.nic.Promiscuous(), stack.CanBePrimaryEndpoint) + addressEndpoint := e.AcquireAssignedAddress(header.IPv6(pkt.NetworkHeader().Slice()).SourceAddress(), e.nic.Promiscuous(), stack.CanBePrimaryEndpoint, true /* readOnly */) if addressEndpoint != nil { - addressEndpoint.DecRef() - // The source address is one of our own, so we never should have gotten // a packet like this unless HandleLocal is false or our NIC is the // loopback interface. @@ -1348,8 +1346,7 @@ func (e *endpoint) handleValidatedPacket(h header.IPv6, pkt *stack.PacketBuffer, // The destination address should be an address we own for us to receive the // packet. Otherwise, attempt to forward the packet. - if addressEndpoint := e.AcquireAssignedAddress(dstAddr, e.nic.Promiscuous(), stack.CanBePrimaryEndpoint); addressEndpoint != nil { - addressEndpoint.DecRef() + if addressEndpoint := e.AcquireAssignedAddress(dstAddr, e.nic.Promiscuous(), stack.CanBePrimaryEndpoint, true /* readOnly */); addressEndpoint != nil { e.deliverPacketLocally(h, pkt, inNICName) } else if e.Forwarding() { e.handleForwardingError(e.forwardUnicastPacket(pkt)) @@ -2036,18 +2033,18 @@ func (e *endpoint) MainAddress() tcpip.AddressWithPrefix { } // AcquireAssignedAddress implements stack.AddressableEndpoint. -func (e *endpoint) AcquireAssignedAddress(localAddr tcpip.Address, allowTemp bool, tempPEB stack.PrimaryEndpointBehavior) stack.AddressEndpoint { +func (e *endpoint) AcquireAssignedAddress(localAddr tcpip.Address, allowTemp bool, tempPEB stack.PrimaryEndpointBehavior, readOnly bool) stack.AddressEndpoint { e.mu.RLock() defer e.mu.RUnlock() - return e.acquireAddressOrCreateTempLocked(localAddr, allowTemp, tempPEB) + return e.acquireAddressOrCreateTempLocked(localAddr, allowTemp, tempPEB, readOnly) } // acquireAddressOrCreateTempLocked is like AcquireAssignedAddress but with // locking requirements. // // Precondition: e.mu must be write locked. -func (e *endpoint) acquireAddressOrCreateTempLocked(localAddr tcpip.Address, allowTemp bool, tempPEB stack.PrimaryEndpointBehavior) stack.AddressEndpoint { - return e.mu.addressableEndpointState.AcquireAssignedAddress(localAddr, allowTemp, tempPEB) +func (e *endpoint) acquireAddressOrCreateTempLocked(localAddr tcpip.Address, allowTemp bool, tempPEB stack.PrimaryEndpointBehavior, readOnly bool) stack.AddressEndpoint { + return e.mu.addressableEndpointState.AcquireAssignedAddress(localAddr, allowTemp, tempPEB, readOnly) } // AcquireOutgoingPrimaryAddress implements stack.AddressableEndpoint. @@ -2369,8 +2366,7 @@ func (p *protocol) findEndpointWithAddress(addr tcpip.Address) *endpoint { defer p.mu.RUnlock() for _, e := range p.mu.eps { - if addressEndpoint := e.AcquireAssignedAddress(addr, false /* allowTemp */, stack.NeverPrimaryEndpoint); addressEndpoint != nil { - addressEndpoint.DecRef() + if addressEndpoint := e.AcquireAssignedAddress(addr, false /* allowTemp */, stack.NeverPrimaryEndpoint, true /* readOnly */); addressEndpoint != nil { return e } } diff --git a/pkg/tcpip/stack/addressable_endpoint_state.go b/pkg/tcpip/stack/addressable_endpoint_state.go index 815c8b121..7695bba92 100644 --- a/pkg/tcpip/stack/addressable_endpoint_state.go +++ b/pkg/tcpip/stack/addressable_endpoint_state.go @@ -537,16 +537,20 @@ func (a *AddressableEndpointState) acquirePrimaryAddressRLocked(remoteAddr, srcH // If there is no matching address, a temporary address will be returned if // allowTemp is true. // +// If readOnly is true, the address will be returned without an extra reference. +// In this case it is not safe to modify the endpoint, only read attributes like +// subnet. +// // Regardless how the address was obtained, it will be acquired before it is // returned. -func (a *AddressableEndpointState) AcquireAssignedAddressOrMatching(localAddr tcpip.Address, f func(AddressEndpoint) bool, allowTemp bool, tempPEB PrimaryEndpointBehavior) AddressEndpoint { +func (a *AddressableEndpointState) AcquireAssignedAddressOrMatching(localAddr tcpip.Address, f func(AddressEndpoint) bool, allowTemp bool, tempPEB PrimaryEndpointBehavior, readOnly bool) AddressEndpoint { lookup := func() *addressState { if addrState, ok := a.endpoints[localAddr]; ok { if !addrState.IsAssigned(allowTemp) { return nil } - if !addrState.TryIncRef() { + if !readOnly && !addrState.TryIncRef() { panic(fmt.Sprintf("failed to increase the reference count for address = %s", addrState.addr)) } @@ -555,7 +559,10 @@ func (a *AddressableEndpointState) AcquireAssignedAddressOrMatching(localAddr tc if f != nil { for _, addrState := range a.endpoints { - if addrState.IsAssigned(allowTemp) && f(addrState) && addrState.TryIncRef() { + if addrState.IsAssigned(allowTemp) && f(addrState) { + if !readOnly && !addrState.TryIncRef() { + continue + } return addrState } } @@ -614,12 +621,22 @@ func (a *AddressableEndpointState) AcquireAssignedAddressOrMatching(localAddr tc if ep == nil { return nil } + if readOnly { + if ep.addressableEndpointState == a { + // Checklocks doesn't understand that we are logically guaranteed to have + // ep.mu locked already. We need to use checklocksignore to appease the + // analyzer. + ep.addressableEndpointState.decAddressRefLocked(ep) // +checklocksignore + } else { + ep.DecRef() + } + } return ep } // AcquireAssignedAddress implements AddressableEndpoint. -func (a *AddressableEndpointState) AcquireAssignedAddress(localAddr tcpip.Address, allowTemp bool, tempPEB PrimaryEndpointBehavior) AddressEndpoint { - return a.AcquireAssignedAddressOrMatching(localAddr, nil, allowTemp, tempPEB) +func (a *AddressableEndpointState) AcquireAssignedAddress(localAddr tcpip.Address, allowTemp bool, tempPEB PrimaryEndpointBehavior, readOnly bool) AddressEndpoint { + return a.AcquireAssignedAddressOrMatching(localAddr, nil, allowTemp, tempPEB, readOnly) } // AcquireOutgoingPrimaryAddress implements AddressableEndpoint. diff --git a/pkg/tcpip/stack/addressable_endpoint_state_test.go b/pkg/tcpip/stack/addressable_endpoint_state_test.go index 1d320c430..1c772890a 100644 --- a/pkg/tcpip/stack/addressable_endpoint_state_test.go +++ b/pkg/tcpip/stack/addressable_endpoint_state_test.go @@ -47,16 +47,14 @@ func TestAddressableEndpointStateCleanup(t *testing.T) { ep.DecRef() } { - ep := s.AcquireAssignedAddress(addr.Address, false /* allowTemp */, stack.NeverPrimaryEndpoint) + ep := s.AcquireAssignedAddress(addr.Address, false /* allowTemp */, stack.NeverPrimaryEndpoint, true /* readOnly */) if ep == nil { t.Fatalf("got s.AcquireAssignedAddress(%s, false, NeverPrimaryEndpoint) = nil, want = non-nil", addr.Address) } - ep.DecRef() } s.Cleanup() - if ep := s.AcquireAssignedAddress(addr.Address, false /* allowTemp */, stack.NeverPrimaryEndpoint); ep != nil { - ep.DecRef() + if ep := s.AcquireAssignedAddress(addr.Address, false /* allowTemp */, stack.NeverPrimaryEndpoint, true /* readOnly */); ep != nil { t.Fatalf("got s.AcquireAssignedAddress(%s, false, NeverPrimaryEndpoint) = %s, want = nil", addr.Address, ep.AddressWithPrefix()) } } diff --git a/pkg/tcpip/stack/forwarding_test.go b/pkg/tcpip/stack/forwarding_test.go index c04573352..f60e97233 100644 --- a/pkg/tcpip/stack/forwarding_test.go +++ b/pkg/tcpip/stack/forwarding_test.go @@ -88,9 +88,8 @@ func (f *fwdTestNetworkEndpoint) HandlePacket(pkt *PacketBuffer) { netHdr := pkt.NetworkHeader().Slice() _, dst := f.proto.ParseAddresses(netHdr) - addressEndpoint := f.AcquireAssignedAddress(dst, f.nic.Promiscuous(), CanBePrimaryEndpoint) + addressEndpoint := f.AcquireAssignedAddress(dst, f.nic.Promiscuous(), CanBePrimaryEndpoint, true /* readOnly */) if addressEndpoint != nil { - addressEndpoint.DecRef() // Dispatch the packet to the transport protocol. f.dispatcher.DeliverTransportPacket(tcpip.TransportProtocolNumber(netHdr[protocolNumberOffset]), pkt) return diff --git a/pkg/tcpip/stack/nic.go b/pkg/tcpip/stack/nic.go index 55a4e76f4..eb298410a 100644 --- a/pkg/tcpip/stack/nic.go +++ b/pkg/tcpip/stack/nic.go @@ -505,7 +505,7 @@ func (n *nic) getAddressOrCreateTempInner(protocol tcpip.NetworkProtocolNumber, return nil } - return addressableEndpoint.AcquireAssignedAddress(address, createTemp, peb) + return addressableEndpoint.AcquireAssignedAddress(address, createTemp, peb, false) } // addAddress adds a new address to n, so that it starts accepting packets diff --git a/pkg/tcpip/stack/registration.go b/pkg/tcpip/stack/registration.go index 8b1003968..519775769 100644 --- a/pkg/tcpip/stack/registration.go +++ b/pkg/tcpip/stack/registration.go @@ -668,10 +668,11 @@ type AddressableEndpoint interface { // that is considered bound to the endpoint, optionally creating a temporary // endpoint if requested and no existing address exists. // - // The returned endpoint's reference count is incremented. + // The returned endpoint's reference count is incremented if readOnly is + // false. // // Returns nil if the specified address is not local to this endpoint. - AcquireAssignedAddress(localAddr tcpip.Address, allowTemp bool, tempPEB PrimaryEndpointBehavior) AddressEndpoint + AcquireAssignedAddress(localAddr tcpip.Address, allowTemp bool, tempPEB PrimaryEndpointBehavior, readOnly bool) AddressEndpoint // AcquireOutgoingPrimaryAddress returns a primary address that may be used as // a source address when sending packets to the passed remote address. diff --git a/pkg/tcpip/stack/stack_test.go b/pkg/tcpip/stack/stack_test.go index 29fd50d73..e8b92ef72 100644 --- a/pkg/tcpip/stack/stack_test.go +++ b/pkg/tcpip/stack/stack_test.go @@ -130,11 +130,10 @@ func (f *fakeNetworkEndpoint) HandlePacket(pkt *stack.PacketBuffer) { netHdr := pkt.NetworkHeader().Slice() dst := tcpip.AddrFromSlice(netHdr[dstAddrOffset:][:header.IPv4AddressSize]) - addressEndpoint := f.AcquireAssignedAddress(dst, f.nic.Promiscuous(), stack.CanBePrimaryEndpoint) + addressEndpoint := f.AcquireAssignedAddress(dst, f.nic.Promiscuous(), stack.CanBePrimaryEndpoint, true /* readOnly */) if addressEndpoint == nil { return } - addressEndpoint.DecRef() f.proto.packetCount[int(dst.AsSlice()[0])%len(f.proto.packetCount)]++