Skip to content

Commit 89d3419

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]>
1 parent 843cd96 commit 89d3419

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
@@ -35,12 +35,13 @@ func (d *driver) Join(ctx context.Context, nid, eid string, sboxKey string, jinf
3535
return err
3636
}
3737

38-
n := d.network(nid)
39-
if n == nil {
40-
return fmt.Errorf("could not find network with id %s", nid)
38+
n, unlock, err := d.lockNetwork(nid)
39+
if err != nil {
40+
return err
4141
}
42+
defer unlock()
4243

43-
ep := n.endpoint(eid)
44+
ep := n.endpoints[eid]
4445
if ep == nil {
4546
return fmt.Errorf("could not find endpoint with id %s", eid)
4647
}
@@ -69,8 +70,6 @@ func (d *driver) Join(ctx context.Context, nid, eid string, sboxKey string, jinf
6970
return fmt.Errorf("network sandbox join failed: %v", err)
7071
}
7172

72-
sbox := n.sandbox()
73-
7473
overlayIfName, containerIfName, err := createVethPair()
7574
if err != nil {
7675
return err
@@ -92,7 +91,7 @@ func (d *driver) Join(ctx context.Context, nid, eid string, sboxKey string, jinf
9291
return err
9392
}
9493

95-
if err = sbox.AddInterface(ctx, overlayIfName, "veth", "", osl.WithMaster(s.brName)); err != nil {
94+
if err = n.sbox.AddInterface(ctx, overlayIfName, "veth", "", osl.WithMaster(s.brName)); err != nil {
9695
return fmt.Errorf("could not add veth pair inside the network sandbox: %v", err)
9796
}
9897

@@ -125,7 +124,9 @@ func (d *driver) Join(ctx context.Context, nid, eid string, sboxKey string, jinf
125124
}
126125
}
127126

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

130131
buf, err := proto.Marshal(&PeerRecord{
131132
EndpointIP: ep.addr.String(),
@@ -198,12 +199,32 @@ func (d *driver) EventNotify(etype driverapi.EventType, nid, tableName, key stri
198199
return
199200
}
200201

201-
if etype == driverapi.Delete {
202-
d.peerDelete(nid, eid, addr, mac, vtep)
202+
n, unlock, err := d.lockNetwork(nid)
203+
if err != nil {
204+
log.G(context.TODO()).WithFields(log.Fields{
205+
"error": err,
206+
"nid": nid,
207+
}).Error("overlay: handling peer event")
203208
return
204209
}
210+
defer unlock()
205211

206-
d.peerAdd(nid, eid, addr, mac, vtep)
212+
var opname string
213+
if etype == driverapi.Delete {
214+
opname = "delete"
215+
err = n.peerDelete(eid, addr, mac, vtep)
216+
} else {
217+
opname = "add"
218+
err = n.peerAdd(eid, addr, mac, vtep)
219+
}
220+
if err != nil {
221+
log.G(context.TODO()).WithFields(log.Fields{
222+
"error": err,
223+
"nid": n.id,
224+
"peer": peer,
225+
"op": opname,
226+
}).Warn("Peer operation failed")
227+
}
207228
}
208229

209230
// Leave method is invoked when a Sandbox detaches from an endpoint.
@@ -212,18 +233,21 @@ func (d *driver) Leave(nid, eid string) error {
212233
return err
213234
}
214235

215-
n := d.network(nid)
216-
if n == nil {
217-
return fmt.Errorf("could not find network with id %s", nid)
236+
n, unlock, err := d.lockNetwork(nid)
237+
if err != nil {
238+
return err
218239
}
240+
defer unlock()
219241

220-
ep := n.endpoint(eid)
242+
ep := n.endpoints[eid]
221243

222244
if ep == nil {
223245
return types.InternalMaskableErrorf("could not find endpoint with id %s", eid)
224246
}
225247

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

228252
n.leaveSandbox()
229253

‎libnetwork/drivers/overlay/ov_endpoint.go‎

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

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

61-
n := d.network(nid)
62-
if n == nil {
63-
return fmt.Errorf("network id %q not found", nid)
42+
n, unlock, err := d.lockNetwork(nid)
43+
if err != nil {
44+
return err
6445
}
46+
defer unlock()
6547

6648
ep := &endpoint{
6749
id: eid,
@@ -85,7 +67,7 @@ func (d *driver) CreateEndpoint(_ context.Context, nid, eid string, ifInfo drive
8567
}
8668
}
8769

88-
n.addEndpoint(ep)
70+
n.endpoints[ep.id] = ep
8971

9072
return nil
9173
}
@@ -97,17 +79,18 @@ func (d *driver) DeleteEndpoint(nid, eid string) error {
9779
return err
9880
}
9981

100-
n := d.network(nid)
101-
if n == nil {
102-
return fmt.Errorf("network id %q not found", nid)
82+
n, unlock, err := d.lockNetwork(nid)
83+
if err != nil {
84+
return err
10385
}
86+
defer unlock()
10487

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

110-
n.deleteEndpoint(eid)
93+
delete(n.endpoints, eid)
11194

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

0 commit comments

Comments
 (0)