From 344f19745f01cb407c714d2a8c95f43022a49ebd Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Wed, 26 Jun 2024 10:31:07 -0700 Subject: [PATCH] Delete pkg/tcpip/stack.UniqueID. There are 2 interfaces in gVisor which are exactly the same: - pkg/tcpip/stack.UniqueID - pkg/sentry/uniqueid.Provider Before this change, both were using the Kernel as the unique number generator. However, we want to decouple the netstack from the kernel. This coupling is causing bugs in restore because netstack needs to be created before the Kernel is restored. So right now, netstack ends up using a "temporary" Kernel which is later destroyed in the restore sequence, but netstack keeps referencing it. Before this change, the unique ID generator in pkg/tcpip/stack.Stack was used for 2 things: 1. Provide NIC IDs which are unique across all network namespaces. 2. Implement stack.TransportEndpoint.UniqueID. (2) is not used anywhere, so deleted it. (1) is overly unique. NIC IDs do not need to be unique across network namespace. So instead of holding a pointer to the kernel, added a NIC ID generator in stack.Stack itself and added NextNICID() function with proper typing. PiperOrigin-RevId: 646997006 --- pkg/abi/linux/netdevice.go | 5 ++++ pkg/sentry/socket/netstack/stack.go | 6 ++--- pkg/tcpip/link/tun/device.go | 2 +- pkg/tcpip/stack/registration.go | 3 --- pkg/tcpip/stack/stack.go | 39 +++++++++------------------- pkg/tcpip/stack/transport_test.go | 7 +---- pkg/tcpip/transport/icmp/endpoint.go | 7 ----- pkg/tcpip/transport/tcp/endpoint.go | 7 ----- pkg/tcpip/transport/udp/endpoint.go | 7 ----- runsc/boot/loader.go | 20 +++++++------- runsc/boot/network.go | 2 +- 11 files changed, 33 insertions(+), 72 deletions(-) diff --git a/pkg/abi/linux/netdevice.go b/pkg/abi/linux/netdevice.go index b30b6d204..0c30c73f8 100644 --- a/pkg/abi/linux/netdevice.go +++ b/pkg/abi/linux/netdevice.go @@ -118,3 +118,8 @@ type EthtoolGetFeaturesBlock struct { Active uint32 NeverChanged uint32 } + +const ( + // LOOPBACK_IFINDEX is defined in include/net/flow.h. + LOOPBACK_IFINDEX = 1 +) diff --git a/pkg/sentry/socket/netstack/stack.go b/pkg/sentry/socket/netstack/stack.go index 3f9f15571..ddd892c25 100644 --- a/pkg/sentry/socket/netstack/stack.go +++ b/pkg/sentry/socket/netstack/stack.go @@ -247,8 +247,8 @@ func (s *Stack) newVeth(ctx context.Context, linkAttrs map[uint16]nlmsg.BytesVie } } ep, peerEP := veth.NewPair(defaultMTU) - id := tcpip.NICID(s.Stack.UniqueID()) - peerID := tcpip.NICID(peerStack.Stack.UniqueID()) + id := s.Stack.NextNICID() + peerID := peerStack.Stack.NextNICID() if ifname == "" { ifname = fmt.Sprintf("veth%d", id) } @@ -293,7 +293,7 @@ func (s *Stack) newBridge(ctx context.Context, linkAttrs map[uint16]nlmsg.BytesV ifname = v.String() } ep := stack.NewBridgeEndpoint(defaultMTU) - id := tcpip.NICID(s.Stack.UniqueID()) + id := s.Stack.NextNICID() err := s.Stack.CreateNICWithOptions(id, ep, stack.NICOptions{ Name: ifname, }) diff --git a/pkg/tcpip/link/tun/device.go b/pkg/tcpip/link/tun/device.go index 4bb74640b..9b0903183 100644 --- a/pkg/tcpip/link/tun/device.go +++ b/pkg/tcpip/link/tun/device.go @@ -140,7 +140,7 @@ func attachOrCreateNIC(s *stack.Stack, name, prefix string, linkCaps stack.LinkE } // 2. Creating a new NIC. - id := tcpip.NICID(s.UniqueID()) + id := s.NextNICID() endpoint := &tunEndpoint{ Endpoint: channel.New(defaultDevOutQueueLen, defaultDevMtu, ""), stack: s, diff --git a/pkg/tcpip/stack/registration.go b/pkg/tcpip/stack/registration.go index b0bba0c59..bd3a66fac 100644 --- a/pkg/tcpip/stack/registration.go +++ b/pkg/tcpip/stack/registration.go @@ -113,9 +113,6 @@ type TransportError interface { // TransportEndpoint is the interface that needs to be implemented by transport // protocol (e.g., tcp, udp) endpoints that can handle packets. type TransportEndpoint interface { - // UniqueID returns an unique ID for this transport endpoint. - UniqueID() uint64 - // HandlePacket is called by the stack when new packets arrive to this // transport endpoint. It sets the packet buffer's transport header. // diff --git a/pkg/tcpip/stack/stack.go b/pkg/tcpip/stack/stack.go index 0b2bf516e..61b91aa06 100644 --- a/pkg/tcpip/stack/stack.go +++ b/pkg/tcpip/stack/stack.go @@ -64,13 +64,6 @@ type ResumableEndpoint interface { Resume() } -// uniqueIDGenerator is a default unique ID generator. -type uniqueIDGenerator atomicbitops.Uint64 - -func (u *uniqueIDGenerator) UniqueID() uint64 { - return ((*atomicbitops.Uint64)(u)).Add(1) -} - var netRawMissingLogger = log.BasicRateLimitedLogger(time.Minute) // Stack is a networking stack, with all supported protocols, NICs, and route @@ -100,9 +93,13 @@ type Stack struct { mu stackRWMutex `state:"nosave"` // +checklocks:mu - nics map[tcpip.NICID]*nic + nics map[tcpip.NICID]*nic + // +checklocks:mu defaultForwardingEnabled map[tcpip.NetworkProtocolNumber]struct{} + // nicIDGen is used to generate NIC IDs. + nicIDGen atomicbitops.Int32 + // cleanupEndpointsMu protects cleanupEndpoints. cleanupEndpointsMu cleanupEndpointsMutex `state:"nosave"` // +checklocks:cleanupEndpointsMu @@ -149,9 +146,6 @@ type Stack struct { // integrator NUD related events. nudDisp NUDDispatcher - // uniqueIDGenerator is a generator of unique identifiers. - uniqueIDGenerator UniqueID - // randomGenerator is an injectable pseudo random generator that can be // used when a random number is required. It must not be used in // security-sensitive contexts. @@ -187,11 +181,6 @@ type Stack struct { tsOffsetSecret uint32 } -// UniqueID is an abstract generator of unique identifiers. -type UniqueID interface { - UniqueID() uint64 -} - // NetworkProtocolFactory instantiates a network protocol. // // NetworkProtocolFactory must not attempt to modify the stack, it may only @@ -225,9 +214,6 @@ type Options struct { // stack (false). HandleLocal bool - // UniqueID is an optional generator of unique identifiers. - UniqueID UniqueID - // NUDConfigs is the default NUD configurations used by interfaces. NUDConfigs NUDConfigurations @@ -353,10 +339,6 @@ func New(opts Options) *Stack { clock = tcpip.NewStdClock() } - if opts.UniqueID == nil { - opts.UniqueID = new(uniqueIDGenerator) - } - if opts.SecureRNG == nil { opts.SecureRNG = cryptorand.Reader } @@ -398,7 +380,6 @@ func New(opts Options) *Stack { icmpRateLimiter: NewICMPRateLimiter(clock), seed: secureRNG.Uint32(), nudConfigs: opts.NUDConfigs, - uniqueIDGenerator: opts.UniqueID, nudDisp: opts.NUDDisp, insecureRNG: insecureRNG, secureRNG: secureRNG, @@ -439,9 +420,13 @@ func New(opts Options) *Stack { return s } -// UniqueID returns a unique identifier. -func (s *Stack) UniqueID() uint64 { - return s.uniqueIDGenerator.UniqueID() +// NextNICID allocates the next available NIC ID and returns it. +func (s *Stack) NextNICID() tcpip.NICID { + next := s.nicIDGen.Add(1) + if next < 0 { + panic("NICID overflow") + } + return tcpip.NICID(next) } // SetNetworkProtocolOption allows configuring individual protocol level diff --git a/pkg/tcpip/stack/transport_test.go b/pkg/tcpip/stack/transport_test.go index 1052396b5..f9b3ab983 100644 --- a/pkg/tcpip/stack/transport_test.go +++ b/pkg/tcpip/stack/transport_test.go @@ -45,7 +45,6 @@ type fakeTransportEndpoint struct { proto *fakeTransportProtocol peerAddr tcpip.Address route *stack.Route - uniqueID uint64 // acceptQueue is non-nil iff bound. acceptQueue []*fakeTransportEndpoint @@ -69,7 +68,7 @@ func (f *fakeTransportEndpoint) SocketOptions() *tcpip.SocketOptions { } func newFakeTransportEndpoint(proto *fakeTransportProtocol, netProto tcpip.NetworkProtocolNumber, s *stack.Stack) tcpip.Endpoint { - ep := &fakeTransportEndpoint{TransportEndpointInfo: stack.TransportEndpointInfo{NetProto: netProto}, proto: proto, uniqueID: s.UniqueID()} + ep := &fakeTransportEndpoint{TransportEndpointInfo: stack.TransportEndpointInfo{NetProto: netProto}, proto: proto} ep.ops.InitHandler(ep, s, tcpip.GetStackSendBufferLimits, tcpip.GetStackReceiveBufferLimits) return ep } @@ -162,10 +161,6 @@ func (f *fakeTransportEndpoint) Connect(addr tcpip.FullAddress) tcpip.Error { return nil } -func (f *fakeTransportEndpoint) UniqueID() uint64 { - return f.uniqueID -} - func (*fakeTransportEndpoint) ConnectEndpoint(e tcpip.Endpoint) tcpip.Error { return nil } diff --git a/pkg/tcpip/transport/icmp/endpoint.go b/pkg/tcpip/transport/icmp/endpoint.go index f411832e0..988604fcd 100644 --- a/pkg/tcpip/transport/icmp/endpoint.go +++ b/pkg/tcpip/transport/icmp/endpoint.go @@ -60,7 +60,6 @@ type endpoint struct { stack *stack.Stack `state:"manual"` transProto tcpip.TransportProtocolNumber waiterQueue *waiter.Queue - uniqueID uint64 net network.Endpoint stats tcpip.TransportEndpointStats ops tcpip.SocketOptions @@ -86,7 +85,6 @@ func newEndpoint(s *stack.Stack, netProto tcpip.NetworkProtocolNumber, transProt stack: s, transProto: transProto, waiterQueue: waiterQueue, - uniqueID: s.UniqueID(), } ep.ops.InitHandler(ep, ep.stack, tcpip.GetStackSendBufferLimits, tcpip.GetStackReceiveBufferLimits) ep.ops.SetSendBufferSize(32*1024, false /* notify */) @@ -110,11 +108,6 @@ func (e *endpoint) WakeupWriters() { e.net.MaybeSignalWritable() } -// UniqueID implements stack.TransportEndpoint.UniqueID. -func (e *endpoint) UniqueID() uint64 { - return e.uniqueID -} - // Abort implements stack.TransportEndpoint.Abort. func (e *endpoint) Abort() { e.Close() diff --git a/pkg/tcpip/transport/tcp/endpoint.go b/pkg/tcpip/transport/tcp/endpoint.go index 9747cee95..03b6f1d9f 100644 --- a/pkg/tcpip/transport/tcp/endpoint.go +++ b/pkg/tcpip/transport/tcp/endpoint.go @@ -367,7 +367,6 @@ type Endpoint struct { stack *stack.Stack `state:"manual"` protocol *protocol `state:"manual"` waiterQueue *waiter.Queue `state:"wait"` - uniqueID uint64 // hardError is meaningful only when state is stateError. It stores the // error to be returned when read/write syscalls are called and the @@ -607,11 +606,6 @@ type Endpoint struct { pmtud tcpip.PMTUDStrategy } -// UniqueID implements stack.TransportEndpoint.UniqueID. -func (e *Endpoint) UniqueID() uint64 { - return e.uniqueID -} - // calculateAdvertisedMSS calculates the MSS to advertise. // // If userMSS is non-zero and is not greater than the maximum possible MSS for @@ -866,7 +860,6 @@ func newEndpoint(s *stack.Stack, protocol *protocol, netProto tcpip.NetworkProto interval: DefaultKeepaliveInterval, count: DefaultKeepaliveCount, }, - uniqueID: s.UniqueID(), ipv4TTL: tcpip.UseDefaultIPv4TTL, ipv6HopLimit: tcpip.UseDefaultIPv6HopLimit, // txHash only determines which outgoing queue to use, so diff --git a/pkg/tcpip/transport/udp/endpoint.go b/pkg/tcpip/transport/udp/endpoint.go index 6c4de0a10..f8e305797 100644 --- a/pkg/tcpip/transport/udp/endpoint.go +++ b/pkg/tcpip/transport/udp/endpoint.go @@ -63,7 +63,6 @@ type endpoint struct { // change throughout the lifetime of the endpoint. stack *stack.Stack `state:"manual"` waiterQueue *waiter.Queue - uniqueID uint64 net network.Endpoint stats tcpip.TransportEndpointStats ops tcpip.SocketOptions @@ -110,7 +109,6 @@ func newEndpoint(s *stack.Stack, netProto tcpip.NetworkProtocolNumber, waiterQue e := &endpoint{ stack: s, waiterQueue: waiterQueue, - uniqueID: s.UniqueID(), } e.ops.InitHandler(e, e.stack, tcpip.GetStackSendBufferLimits, tcpip.GetStackReceiveBufferLimits) e.ops.SetMulticastLoop(true) @@ -137,11 +135,6 @@ func (e *endpoint) WakeupWriters() { e.net.MaybeSignalWritable() } -// UniqueID implements stack.TransportEndpoint. -func (e *endpoint) UniqueID() uint64 { - return e.uniqueID -} - func (e *endpoint) LastError() tcpip.Error { e.lastErrorMu.Lock() defer e.lastErrorMu.Unlock() diff --git a/runsc/boot/loader.go b/runsc/boot/loader.go index aba608b36..1aaa1f00c 100644 --- a/runsc/boot/loader.go +++ b/runsc/boot/loader.go @@ -503,7 +503,7 @@ func New(args Args) (*Loader, error) { return nil, fmt.Errorf("getting root credentials") } // Create root network namespace/stack. - netns, err := newRootNetworkNamespace(args.Conf, tk, l.k, creds.UserNamespace) + netns, err := newRootNetworkNamespace(args.Conf, tk, creds.UserNamespace) if err != nil { return nil, fmt.Errorf("creating network: %w", err) } @@ -1381,7 +1381,7 @@ func (l *Loader) WaitExit() linux.WaitStatus { return l.k.GlobalInit().ExitStatus() } -func newRootNetworkNamespace(conf *config.Config, clock tcpip.Clock, uniqueID stack.UniqueID, userns *auth.UserNamespace) (*inet.Namespace, error) { +func newRootNetworkNamespace(conf *config.Config, clock tcpip.Clock, userns *auth.UserNamespace) (*inet.Namespace, error) { // Create an empty network stack because the network namespace may be empty at // this point. Netns is configured before Run() is called. Netstack is // configured using a control uRPC message. Host network is configured inside @@ -1398,13 +1398,12 @@ func newRootNetworkNamespace(conf *config.Config, clock tcpip.Clock, uniqueID st return inet.NewRootNamespace(hostinet.NewStack(), nil, userns), nil case config.NetworkNone, config.NetworkSandbox: - s, err := newEmptySandboxNetworkStack(clock, uniqueID, conf.AllowPacketEndpointWrite) + s, err := newEmptySandboxNetworkStack(clock, conf.AllowPacketEndpointWrite) if err != nil { return nil, err } creator := &sandboxNetstackCreator{ clock: clock, - uniqueID: uniqueID, allowPacketEndpointWrite: conf.AllowPacketEndpointWrite, } return inet.NewRootNamespace(s, creator, userns), nil @@ -1415,7 +1414,7 @@ func newRootNetworkNamespace(conf *config.Config, clock tcpip.Clock, uniqueID st } -func newEmptySandboxNetworkStack(clock tcpip.Clock, uniqueID stack.UniqueID, allowPacketEndpointWrite bool) (inet.Stack, error) { +func newEmptySandboxNetworkStack(clock tcpip.Clock, allowPacketEndpointWrite bool) (*netstack.Stack, error) { netProtos := []stack.NetworkProtocolFactory{ipv4.NewProtocol, ipv6.NewProtocol, arp.NewProtocol} transProtos := []stack.TransportProtocolFactory{ tcp.NewProtocol, @@ -1433,7 +1432,6 @@ func newEmptySandboxNetworkStack(clock tcpip.Clock, uniqueID stack.UniqueID, all // privileges. RawFactory: raw.EndpointFactory{}, AllowPacketEndpointWrite: allowPacketEndpointWrite, - UniqueID: uniqueID, DefaultIPTables: netfilter.DefaultLinuxTables, })} @@ -1472,20 +1470,22 @@ func newEmptySandboxNetworkStack(clock tcpip.Clock, uniqueID stack.UniqueID, all // +stateify savable type sandboxNetstackCreator struct { clock tcpip.Clock - uniqueID stack.UniqueID allowPacketEndpointWrite bool } // CreateStack implements kernel.NetworkStackCreator.CreateStack. func (f *sandboxNetstackCreator) CreateStack() (inet.Stack, error) { - s, err := newEmptySandboxNetworkStack(f.clock, f.uniqueID, f.allowPacketEndpointWrite) + s, err := newEmptySandboxNetworkStack(f.clock, f.allowPacketEndpointWrite) if err != nil { return nil, err } // Setup loopback. - n := &Network{Stack: s.(*netstack.Stack).Stack} - nicID := tcpip.NICID(f.uniqueID.UniqueID()) + n := &Network{Stack: s.Stack} + nicID := s.Stack.NextNICID() + if nicID != linux.LOOPBACK_IFINDEX { + return nil, fmt.Errorf("loopback device should always have index %d, got %d", linux.LOOPBACK_IFINDEX, nicID) + } link := DefaultLoopbackLink linkEP := ethernet.New(loopback.New()) opts := stack.NICOptions{ diff --git a/runsc/boot/network.go b/runsc/boot/network.go index 823193b52..dd68df57b 100644 --- a/runsc/boot/network.go +++ b/runsc/boot/network.go @@ -255,7 +255,7 @@ func (n *Network) CreateLinksAndRoutes(args *CreateLinksAndRoutesArgs, _ *struct // Loopback normally appear before other interfaces. for _, link := range args.LoopbackLinks { - nicID = tcpip.NICID(n.Stack.UniqueID()) + nicID = n.Stack.NextNICID() nicids[link.Name] = nicID linkEP := ethernet.New(loopback.New())