From b64997ea82a6094c5d4642a577fa3806afa6e9dd Mon Sep 17 00:00:00 2001 From: Chris Telfer Date: Thu, 26 Apr 2018 17:21:19 -0400 Subject: [PATCH 1/3] Fix race conditions in overlay network driver The overlay network driver is not properly using it's mutexes or sync.Onces. It made the classic mistake of not holding a lock through various read-modify-write operations. This can result in inconsistent state storage leading to more catastrophic issues. This patch attempts to maintain the previous semantics while holding the driver lock through operations that are read-modify-write of the driver's network state. One example of this race would be if two goroutines tried to invoke d.network() after the network ID was removed from the table. Both would try to reinstall it causing the "once" to get reinitialized twice without any lock protection. This could then lead to the "once" getting invoked twice on the same network. Furthermore, the changes to one of these network structures gets effectively discarded. It's also the case, that because there would be two simultaneous instances of the network, the various network Lock() invocations would be meaningless for race prevention. Signed-off-by: Chris Telfer --- libnetwork/drivers/overlay/ov_network.go | 55 ++++++++++++++---------- 1 file changed, 32 insertions(+), 23 deletions(-) diff --git a/libnetwork/drivers/overlay/ov_network.go b/libnetwork/drivers/overlay/ov_network.go index 0be94a0708..9c83127d4f 100644 --- a/libnetwork/drivers/overlay/ov_network.go +++ b/libnetwork/drivers/overlay/ov_network.go @@ -203,6 +203,12 @@ func (d *driver) CreateNetwork(id string, option map[string]interface{}, nInfo d n.subnets = append(n.subnets, s) } + d.Lock() + defer d.Unlock() + if d.networks[n.id] != nil { + return fmt.Errorf("attempt to create overlay network %v that already exists", n.id) + } + if err := n.writeToStore(); err != nil { return fmt.Errorf("failed to update data store for network %v: %v", n.id, err) } @@ -217,11 +223,13 @@ func (d *driver) CreateNetwork(id string, option map[string]interface{}, nInfo d if nInfo != nil { if err := nInfo.TableEventRegister(ovPeerTable, driverapi.EndpointObject); err != nil { + // XXX Undo writeToStore? No method to so. Why? return err } } - d.addNetwork(n) + d.networks[id] = n + return nil } @@ -235,7 +243,15 @@ func (d *driver) DeleteNetwork(nid string) error { return err } - n := d.network(nid) + d.Lock() + defer d.Unlock() + + // This is similar to d.network(), but we need to keep holding the lock + // until we are done removing this network. + n, ok := d.networks[nid] + if !ok { + n = d.restoreNetworkFromStore(nid) + } if n == nil { return fmt.Errorf("could not find network with id %s", nid) } @@ -255,7 +271,7 @@ func (d *driver) DeleteNetwork(nid string) error { } // flush the peerDB entries d.peerFlush(nid) - d.deleteNetwork(nid) + delete(d.networks, nid) vnis, err := n.releaseVxlanID() if err != nil { @@ -805,32 +821,25 @@ func (n *network) watchMiss(nlSock *nl.NetlinkSocket, nsPath string) { } } -func (d *driver) addNetwork(n *network) { - d.Lock() - d.networks[n.id] = n - d.Unlock() -} - -func (d *driver) deleteNetwork(nid string) { - d.Lock() - delete(d.networks, nid) - d.Unlock() +// Restore a network from the store to the driver if it is present. +// Must be called with the driver locked! +func (d *driver) restoreNetworkFromStore(nid string) *network { + n := d.getNetworkFromStore(nid) + if n != nil { + n.driver = d + n.endpoints = endpointTable{} + n.once = &sync.Once{} + d.networks[nid] = n + } + return n } func (d *driver) network(nid string) *network { d.Lock() + defer d.Unlock() n, ok := d.networks[nid] - d.Unlock() if !ok { - n = d.getNetworkFromStore(nid) - if n != nil { - n.driver = d - n.endpoints = endpointTable{} - n.once = &sync.Once{} - d.Lock() - d.networks[nid] = n - d.Unlock() - } + n = d.restoreNetworkFromStore(nid) } return n From 40b55d233688165411365b7ef035ff722257e5c8 Mon Sep 17 00:00:00 2001 From: Chris Telfer Date: Fri, 27 Apr 2018 10:48:48 -0400 Subject: [PATCH 2/3] Remove race condition from ovnmanager This one is probably not critical. The worst that seems like could happen would be if 2 deletes occur at the same time (one of which should be an error): 1. network gets read from the map by delete-1 2. network gets read from the map by delete-2 3. delete-1 releases the network VNI 4. network create arrives at the driver and allocates the now free VNI 5. delete-2 releases the network VNI (error: it's been reallocated!) 6. both networks remove the VNI from the map Part 6 could also become an issue if there were a simultaneous create for the network at the same time. This leads to the modification of the NewNetwork() method which now checks for an existing network before adding it to the map. Signed-off-by: Chris Telfer --- libnetwork/drivers/overlay/ovmanager/ovmanager.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/libnetwork/drivers/overlay/ovmanager/ovmanager.go b/libnetwork/drivers/overlay/ovmanager/ovmanager.go index 58cc687d4f..12deb22e44 100644 --- a/libnetwork/drivers/overlay/ovmanager/ovmanager.go +++ b/libnetwork/drivers/overlay/ovmanager/ovmanager.go @@ -125,8 +125,12 @@ func (d *driver) NetworkAllocate(id string, option map[string]string, ipV4Data, opts[netlabel.OverlayVxlanIDList] = val d.Lock() + defer d.Unlock() + if _, ok := d.networks[id]; ok { + n.releaseVxlanID() + return nil, fmt.Errorf("network %s already exists", id) + } d.networks[id] = n - d.Unlock() return opts, nil } @@ -137,8 +141,8 @@ func (d *driver) NetworkFree(id string) error { } d.Lock() + defer d.Unlock() n, ok := d.networks[id] - d.Unlock() if !ok { return fmt.Errorf("overlay network with id %s not found", id) @@ -147,9 +151,7 @@ func (d *driver) NetworkFree(id string) error { // Release all vxlan IDs in one shot. n.releaseVxlanID() - d.Lock() delete(d.networks, id) - d.Unlock() return nil } From c97bb416200ee2632f2c3820841e85dd5e9cdb96 Mon Sep 17 00:00:00 2001 From: Chris Telfer Date: Fri, 27 Apr 2018 11:24:36 -0400 Subject: [PATCH 3/3] Remove race in encrypted overlay key update Multiple simultaneous updates here would leave the driver in a very inconsistent state. The disadvantage to this change is that it requires holding the driver lock while reprogramming the keys. Signed-off-by: Chris Telfer --- libnetwork/drivers/overlay/encryption.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/libnetwork/drivers/overlay/encryption.go b/libnetwork/drivers/overlay/encryption.go index 802d7bc36d..bcae0bd4e5 100644 --- a/libnetwork/drivers/overlay/encryption.go +++ b/libnetwork/drivers/overlay/encryption.go @@ -438,7 +438,7 @@ func (d *driver) setKeys(keys []*key) error { d.keys = keys d.secMap = &encrMap{nodes: map[string][]*spi{}} d.Unlock() - logrus.Debugf("Initial encryption keys: %v", d.keys) + logrus.Debugf("Initial encryption keys: %v", keys) return nil } @@ -458,6 +458,8 @@ func (d *driver) updateKeys(newKey, primary, pruneKey *key) error { ) d.Lock() + defer d.Unlock() + // add new if newKey != nil { d.keys = append(d.keys, newKey) @@ -471,7 +473,6 @@ func (d *driver) updateKeys(newKey, primary, pruneKey *key) error { delIdx = i } } - d.Unlock() if (newKey != nil && newIdx == -1) || (primary != nil && priIdx == -1) || @@ -480,17 +481,18 @@ func (d *driver) updateKeys(newKey, primary, pruneKey *key) error { "(newIdx,priIdx,delIdx):(%d, %d, %d)", newIdx, priIdx, delIdx) } + if priIdx != -1 && priIdx == delIdx { + return types.BadRequestErrorf("attempting to both make a key (index %d) primary and delete it", priIdx) + } + d.secMapWalk(func(rIPs string, spis []*spi) ([]*spi, bool) { rIP := net.ParseIP(rIPs) return updateNodeKey(lIP, aIP, rIP, spis, d.keys, newIdx, priIdx, delIdx), false }) - d.Lock() // swap primary if priIdx != -1 { - swp := d.keys[0] - d.keys[0] = d.keys[priIdx] - d.keys[priIdx] = swp + d.keys[0], d.keys[priIdx] = d.keys[priIdx], d.keys[0] } // prune if delIdx != -1 { @@ -499,7 +501,6 @@ func (d *driver) updateKeys(newKey, primary, pruneKey *key) error { } d.keys = append(d.keys[:delIdx], d.keys[delIdx+1:]...) } - d.Unlock() logrus.Debugf("Updated: %v", d.keys)