Skip to content

Commit d60c71a

Browse files
corherecorhere
committed
libnetwork/d/overlay: fix logical race conditions
The concurrency control in the overlay driver is logically unsound. While the use of mutexes is sufficient to prevent data races -- violations of the Go memory model -- many operations which need to be atomic are performed with unbounded concurrency. Overhaul the use of locks in the overlay network driver. Implement sound locking at the network granularity: operations may proceed concurrently iff they are being applied to distinct networks. Push the responsibility of locking up to the code which calls methods or accesses struct fields to avoid deadlock situations like we had previously with d.initSandboxPeerDB() and to make the code easier to reason about. Each overlay network has a distinct peer db. The NetworkDB watch for the overlay peer table for the network will only start after (*driver).CreateNetwork returns and will be stopped before libnetwork calls (*driver).DeleteNetwork, therefore the lifetime of the peer db for a network is constrained to the lifetime of the network itself. Yet the peer db for a network is tracked in a dedicated map, separately from the network objects themselves. This has resulted in a parallel set of mutexes to manage concurrency of the peer db distinct from the mutexes for the driver and networks. Move the peer db for a network into a field of the network struct and guard it from concurrent access using the per-network lock. Move the methods for manipulating the peer db into the network struct so that the methods can only be called if the caller has a reference to the network object. Network creation and deletion are synchronized using the driver-scope mutex, but some of the kernel programming is performed outside of the critical section. It is possible for network deletion to race with recreating the network, interleaving the kernel programming for the network creation and deletion, resulting in inconsistent kernel state. Parallelize network creation and deletion soundly. Use a double-checked locking scheme to soundly handle the case of concurrent CreateNetwork and DeleteNetwork for the same network id without blocking operations on other networks. Synchronize operations on a network so that operations on the network such as adding a neighbor to the peer db are performed atomically, not interleaved with deleting the network. Signed-off-by: Cory Snider <[email protected]> (cherry picked from commit 89d3419) Signed-off-by: Cory Snider <[email protected]>
1 parent ad54b8f commit d60c71a

5 files changed

Lines changed: 221 additions & 272 deletions

File tree

‎libnetwork/drivers/overlay/joinleave.go‎

Lines changed: 40 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,13 @@ func (d *driver) Join(nid, eid string, sboxKey string, jinfo driverapi.JoinInfo,
2525
return err
2626
}
2727

28-
n := d.network(nid)
29-
if n == nil {
30-
return fmt.Errorf("could not find network with id %s", nid)
28+
n, unlock, err := d.lockNetwork(nid)
29+
if err != nil {
30+
return err
3131
}
32+
defer unlock()
3233

33-
ep := n.endpoint(eid)
34+
ep := n.endpoints[eid]
3435
if ep == nil {
3536
return fmt.Errorf("could not find endpoint with id %s", eid)
3637
}
@@ -59,8 +60,6 @@ func (d *driver) Join(nid, eid string, sboxKey string, jinfo driverapi.JoinInfo,
5960
return fmt.Errorf("network sandbox join failed: %v", err)
6061
}
6162

62-
sbox := n.sandbox()
63-
6463
overlayIfName, containerIfName, err := createVethPair()
6564
if err != nil {
6665
return err
@@ -82,7 +81,7 @@ func (d *driver) Join(nid, eid string, sboxKey string, jinfo driverapi.JoinInfo,
8281
return err
8382
}
8483

85-
if err = sbox.AddInterface(overlayIfName, "veth", osl.WithMaster(s.brName)); err != nil {
84+
if err = n.sbox.AddInterface(overlayIfName, "veth", osl.WithMaster(s.brName)); err != nil {
8685
return fmt.Errorf("could not add veth pair inside the network sandbox: %v", err)
8786
}
8887

@@ -115,7 +114,9 @@ func (d *driver) Join(nid, eid string, sboxKey string, jinfo driverapi.JoinInfo,
115114
}
116115
}
117116

118-
d.peerAdd(nid, eid, ep.addr, ep.mac, netip.Addr{})
117+
if err := n.peerAdd(eid, ep.addr, ep.mac, netip.Addr{}); err != nil {
118+
return fmt.Errorf("overlay: failed to add local endpoint to network peer db: %w", err)
119+
}
119120

120121
buf, err := proto.Marshal(&PeerRecord{
121122
EndpointIP: ep.addr.String(),
@@ -188,12 +189,32 @@ func (d *driver) EventNotify(etype driverapi.EventType, nid, tableName, key stri
188189
return
189190
}
190191

191-
if etype == driverapi.Delete {
192-
d.peerDelete(nid, eid, addr, mac, vtep)
192+
n, unlock, err := d.lockNetwork(nid)
193+
if err != nil {
194+
log.G(context.TODO()).WithFields(log.Fields{
195+
"error": err,
196+
"nid": nid,
197+
}).Error("overlay: handling peer event")
193198
return
194199
}
200+
defer unlock()
195201

196-
d.peerAdd(nid, eid, addr, mac, vtep)
202+
var opname string
203+
if etype == driverapi.Delete {
204+
opname = "delete"
205+
err = n.peerDelete(eid, addr, mac, vtep)
206+
} else {
207+
opname = "add"
208+
err = n.peerAdd(eid, addr, mac, vtep)
209+
}
210+
if err != nil {
211+
log.G(context.TODO()).WithFields(log.Fields{
212+
"error": err,
213+
"nid": n.id,
214+
"peer": peer,
215+
"op": opname,
216+
}).Warn("Peer operation failed")
217+
}
197218
}
198219

199220
// Leave method is invoked when a Sandbox detaches from an endpoint.
@@ -202,18 +223,21 @@ func (d *driver) Leave(nid, eid string) error {
202223
return err
203224
}
204225

205-
n := d.network(nid)
206-
if n == nil {
207-
return fmt.Errorf("could not find network with id %s", nid)
226+
n, unlock, err := d.lockNetwork(nid)
227+
if err != nil {
228+
return err
208229
}
230+
defer unlock()
209231

210-
ep := n.endpoint(eid)
232+
ep := n.endpoints[eid]
211233

212234
if ep == nil {
213235
return types.InternalMaskableErrorf("could not find endpoint with id %s", eid)
214236
}
215237

216-
d.peerDelete(nid, eid, ep.addr, ep.mac, netip.Addr{})
238+
if err := n.peerDelete(eid, ep.addr, ep.mac, netip.Addr{}); err != nil {
239+
return fmt.Errorf("overlay: failed to delete local endpoint eid:%s from network peer db: %w", eid, err)
240+
}
217241

218242
n.leaveSandbox()
219243

‎libnetwork/drivers/overlay/ov_endpoint.go‎

Lines changed: 11 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -25,25 +25,6 @@ type endpoint struct {
2525
addr netip.Prefix
2626
}
2727

28-
func (n *network) endpoint(eid string) *endpoint {
29-
n.mu.Lock()
30-
defer n.mu.Unlock()
31-
32-
return n.endpoints[eid]
33-
}
34-
35-
func (n *network) addEndpoint(ep *endpoint) {
36-
n.mu.Lock()
37-
n.endpoints[ep.id] = ep
38-
n.mu.Unlock()
39-
}
40-
41-
func (n *network) deleteEndpoint(eid string) {
42-
n.mu.Lock()
43-
delete(n.endpoints, eid)
44-
n.mu.Unlock()
45-
}
46-
4728
func (d *driver) CreateEndpoint(nid, eid string, ifInfo driverapi.InterfaceInfo, epOptions map[string]interface{}) error {
4829
var err error
4930
if err = validateID(nid, eid); err != nil {
@@ -57,10 +38,11 @@ func (d *driver) CreateEndpoint(nid, eid string, ifInfo driverapi.InterfaceInfo,
5738
return err
5839
}
5940

60-
n := d.network(nid)
61-
if n == nil {
62-
return fmt.Errorf("network id %q not found", nid)
41+
n, unlock, err := d.lockNetwork(nid)
42+
if err != nil {
43+
return err
6344
}
45+
defer unlock()
6446

6547
ep := &endpoint{
6648
id: eid,
@@ -84,7 +66,7 @@ func (d *driver) CreateEndpoint(nid, eid string, ifInfo driverapi.InterfaceInfo,
8466
}
8567
}
8668

87-
n.addEndpoint(ep)
69+
n.endpoints[ep.id] = ep
8870

8971
return nil
9072
}
@@ -96,17 +78,18 @@ func (d *driver) DeleteEndpoint(nid, eid string) error {
9678
return err
9779
}
9880

99-
n := d.network(nid)
100-
if n == nil {
101-
return fmt.Errorf("network id %q not found", nid)
81+
n, unlock, err := d.lockNetwork(nid)
82+
if err != nil {
83+
return err
10284
}
85+
defer unlock()
10386

104-
ep := n.endpoint(eid)
87+
ep := n.endpoints[eid]
10588
if ep == nil {
10689
return fmt.Errorf("endpoint id %q not found", eid)
10790
}
10891

109-
n.deleteEndpoint(eid)
92+
delete(n.endpoints, eid)
11093

11194
if ep.ifName == "" {
11295
return nil

0 commit comments

Comments
 (0)