Skip to content

Commit 75a2440

Browse files
smirasmira
committed
fix: recursive RLock() mutex acquision
There were at least two code paths which might acquire `.RLock()` recursively: 1. `Setup*()` -> `createResult()` 2. `Setup*()` -> `Networks()`. On its own, it's not a problem, but if `.Load()` is called concurrently, it might do `.Lock()` in between recursive `.RLock()`s, which would lead to a deadlock. See #125 Fix by introducing a contract on the way mutex is acquired. Add a test facility to verify for such mistakes via build tags: * `go test -race` or `go test -tags deadlocks` activates deadlock detection Signed-off-by: Andrey Smirnov <[email protected]>
1 parent 642f1ce commit 75a2440

11 files changed

Lines changed: 101 additions & 56 deletions

File tree

‎cni.go‎

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,10 @@ type libcni struct {
8080
cniConfig cnilibrary.CNI
8181
networkCount int // minimum network plugin configurations needed to initialize cni
8282
networks []*Network
83-
sync.RWMutex
83+
// Mutex contract:
84+
// - lock in public methods: write lock when mutating the state, read lock when reading the state.
85+
// - never lock in private methods.
86+
RWMutex
8487
}
8588

8689
func defaultCNIConfig() *libcni {
@@ -135,11 +138,11 @@ func (c *libcni) Load(opts ...Opt) error {
135138

136139
// Status returns the status of CNI initialization.
137140
func (c *libcni) Status() error {
141+
c.RLock()
142+
defer c.RUnlock()
138143
if err := c.ready(); err != nil {
139144
return err
140145
}
141-
c.RLock()
142-
defer c.RUnlock()
143146
// STATUS is only called for CNI Version 1.1.0 or greater. It is ignored for previous versions.
144147
for _, v := range c.networks {
145148
err := c.cniConfig.GetStatusNetworkList(context.Background(), v.config)
@@ -162,11 +165,11 @@ func (c *libcni) Networks() []*Network {
162165

163166
// Setup setups the network in the namespace and returns a Result
164167
func (c *libcni) Setup(ctx context.Context, id string, path string, opts ...NamespaceOpts) (*Result, error) {
168+
c.RLock()
169+
defer c.RUnlock()
165170
if err := c.ready(); err != nil {
166171
return nil, err
167172
}
168-
c.RLock()
169-
defer c.RUnlock()
170173
ns, err := newNamespace(id, path, opts...)
171174
if err != nil {
172175
return nil, err
@@ -180,11 +183,11 @@ func (c *libcni) Setup(ctx context.Context, id string, path string, opts ...Name
180183

181184
// SetupSerially setups the network in the namespace and returns a Result
182185
func (c *libcni) SetupSerially(ctx context.Context, id string, path string, opts ...NamespaceOpts) (*Result, error) {
186+
c.RLock()
187+
defer c.RUnlock()
183188
if err := c.ready(); err != nil {
184189
return nil, err
185190
}
186-
c.RLock()
187-
defer c.RUnlock()
188191
ns, err := newNamespace(id, path, opts...)
189192
if err != nil {
190193
return nil, err
@@ -198,7 +201,7 @@ func (c *libcni) SetupSerially(ctx context.Context, id string, path string, opts
198201

199202
func (c *libcni) attachNetworksSerially(ctx context.Context, ns *Namespace) ([]*types100.Result, error) {
200203
var results []*types100.Result
201-
for _, network := range c.Networks() {
204+
for _, network := range c.networks {
202205
r, err := network.Attach(ctx, ns)
203206
if err != nil {
204207
return nil, err
@@ -223,15 +226,15 @@ func asynchAttach(ctx context.Context, index int, n *Network, ns *Namespace, wg
223226
func (c *libcni) attachNetworks(ctx context.Context, ns *Namespace) ([]*types100.Result, error) {
224227
var wg sync.WaitGroup
225228
var firstError error
226-
results := make([]*types100.Result, len(c.Networks()))
229+
results := make([]*types100.Result, len(c.networks))
227230
rc := make(chan asynchAttachResult)
228231

229-
for i, network := range c.Networks() {
232+
for i, network := range c.networks {
230233
wg.Add(1)
231234
go asynchAttach(ctx, i, network, ns, &wg, rc)
232235
}
233236

234-
for range c.Networks() {
237+
for range c.networks {
235238
rs := <-rc
236239
if rs.err != nil && firstError == nil {
237240
firstError = rs.err
@@ -245,16 +248,16 @@ func (c *libcni) attachNetworks(ctx context.Context, ns *Namespace) ([]*types100
245248

246249
// Remove removes the network config from the namespace
247250
func (c *libcni) Remove(ctx context.Context, id string, path string, opts ...NamespaceOpts) error {
251+
c.RLock()
252+
defer c.RUnlock()
248253
if err := c.ready(); err != nil {
249254
return err
250255
}
251-
c.RLock()
252-
defer c.RUnlock()
253256
ns, err := newNamespace(id, path, opts...)
254257
if err != nil {
255258
return err
256259
}
257-
for _, network := range c.Networks() {
260+
for _, network := range c.networks {
258261
if err := network.Remove(ctx, ns); err != nil {
259262
// Based on CNI spec v0.7.0, empty network namespace is allowed to
260263
// do best effort cleanup. However, it is not handled consistently
@@ -275,16 +278,16 @@ func (c *libcni) Remove(ctx context.Context, id string, path string, opts ...Nam
275278

276279
// Check checks if the network is still in desired state
277280
func (c *libcni) Check(ctx context.Context, id string, path string, opts ...NamespaceOpts) error {
281+
c.RLock()
282+
defer c.RUnlock()
278283
if err := c.ready(); err != nil {
279284
return err
280285
}
281-
c.RLock()
282-
defer c.RUnlock()
283286
ns, err := newNamespace(id, path, opts...)
284287
if err != nil {
285288
return err
286289
}
287-
for _, network := range c.Networks() {
290+
for _, network := range c.networks {
288291
err := network.Check(ctx, ns)
289292
if err != nil {
290293
return err
@@ -329,8 +332,6 @@ func (c *libcni) reset() {
329332
}
330333

331334
func (c *libcni) ready() error {
332-
c.RLock()
333-
defer c.RUnlock()
334335
if len(c.networks) < c.networkCount {
335336
return ErrCNINotInitialized
336337
}

‎cni_test.go‎

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -35,12 +35,13 @@ import (
3535
// TestLibCNIType020 tests the cni version 0.2.0 plugin
3636
// config and parses the result into structured data
3737
func TestLibCNIType020(t *testing.T) {
38+
t.Parallel()
39+
3840
// Get the default CNI config
3941
l := defaultCNIConfig()
4042

4143
// Create a fake cni config directory and file
42-
cniDir, confDir := makeFakeCNIConfig(t)
43-
defer tearDownCNIConfig(t, cniDir)
44+
_, confDir := makeFakeCNIConfig(t)
4445
l.pluginConfDir = confDir
4546
// Set the minimum network count as 2 for this test
4647
l.networkCount = 2
@@ -112,11 +113,12 @@ func TestLibCNIType020(t *testing.T) {
112113
// TestLibCNIType040 tests the cni version 0.4.0 plugin
113114
// config and parses the result into structured data
114115
func TestLibCNIType040(t *testing.T) {
116+
t.Parallel()
117+
115118
// Get the default CNI config
116119
l := defaultCNIConfig()
117120
// Create a fake cni config directory and file
118-
cniDir, confDir := makeFakeCNIConfig(t)
119-
defer tearDownCNIConfig(t, cniDir)
121+
_, confDir := makeFakeCNIConfig(t)
120122
l.pluginConfDir = confDir
121123
// Set the minimum network count as 2 for this test
122124
l.networkCount = 2
@@ -200,11 +202,12 @@ func TestLibCNIType040(t *testing.T) {
200202
// TestLibCNIType100 tests the cni version 1.0.0 plugin
201203
// config and parses the result into structured data
202204
func TestLibCNIType100(t *testing.T) {
205+
t.Parallel()
206+
203207
// Get the default CNI config
204208
l := defaultCNIConfig()
205209
// Create a fake cni config directory and file
206-
cniDir, confDir := makeFakeCNIConfig(t)
207-
defer tearDownCNIConfig(t, cniDir)
210+
_, confDir := makeFakeCNIConfig(t)
208211
l.pluginConfDir = confDir
209212
// Set the minimum network count as 2 for this test
210213
l.networkCount = 2
@@ -290,11 +293,12 @@ func TestLibCNIType100(t *testing.T) {
290293
// TestLibCNIType120 tests the cni version 1.1.0 plugin
291294
// config and parses the result into structured data
292295
func TestLibCNIType120(t *testing.T) {
296+
t.Parallel()
297+
293298
// Get the default CNI config
294299
l := defaultCNIConfig()
295300
// Create a fake cni config directory and file
296-
cniDir, confDir := buildFakeConfig(t)
297-
defer tearDownCNIConfig(t, cniDir)
301+
_, confDir := buildFakeConfig(t)
298302
l.pluginConfDir = confDir
299303
// Set the minimum network count as 2 for this test
300304
l.networkCount = 2
@@ -382,11 +386,12 @@ func TestLibCNIType120(t *testing.T) {
382386
}
383387

384388
func TestLibCNIType120FailStatus(t *testing.T) {
389+
t.Parallel()
390+
385391
// Get the default CNI config
386392
l := defaultCNIConfig()
387393
// Create a fake cni config directory and file
388-
cniDir, confDir := buildFakeConfig(t)
389-
defer tearDownCNIConfig(t, cniDir)
394+
_, confDir := buildFakeConfig(t)
390395
l.pluginConfDir = confDir
391396
// Set the minimum network count as 2 for this test
392397
l.networkCount = 2

‎deprecated.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,5 +30,7 @@ type CNIResult = Result //revive:disable // type name will be used as cni.CNIRes
3030
// results fails, or if a network could not be found.
3131
// Deprecated: do not use
3232
func (c *libcni) GetCNIResultFromResults(results []*types100.Result) (*Result, error) {
33+
c.RLock()
34+
defer c.RUnlock()
3335
return c.createResult(results)
3436
}

‎go.mod‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,13 @@ go 1.21
44

55
require (
66
github.com/containernetworking/cni v1.2.2
7+
github.com/sasha-s/go-deadlock v0.3.5
78
github.com/stretchr/testify v1.8.0
89
)
910

1011
require (
1112
github.com/davecgh/go-spew v1.1.1 // indirect
13+
github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7 // indirect
1214
github.com/pmezard/go-difflib v1.0.0 // indirect
1315
github.com/stretchr/objx v0.4.0 // indirect
1416
gopkg.in/yaml.v3 v3.0.1 // indirect

‎go.sum‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,12 @@ github.com/onsi/ginkgo/v2 v2.19.0 h1:9Cnnf7UHo57Hy3k6/m5k3dRfGTMXGvxhHFvkDTCTpvA
1515
github.com/onsi/ginkgo/v2 v2.19.0/go.mod h1:rlwLi9PilAFJ8jCg9UE1QP6VBpd6/xj3SRC0d6TU0To=
1616
github.com/onsi/gomega v1.33.1 h1:dsYjIxxSR755MDmKVsaFQTE22ChNBcuuTWgkUDSubOk=
1717
github.com/onsi/gomega v1.33.1/go.mod h1:U4R44UsT+9eLIaYRB2a5qajjtQYn0hauxvRm16AVYg0=
18+
github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7 h1:Dx7Ovyv/SFnMFw3fD4oEoeorXc6saIiQ23LrGLth0Gw=
19+
github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7/go.mod h1:pxMtw7cyUw6B2bRH0ZBANSPg+AoSud1I1iyJHI69jH4=
1820
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
1921
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
22+
github.com/sasha-s/go-deadlock v0.3.5 h1:tNCOEEDG6tBqrNDOX35j/7hL5FcFViG6awUGROb2NsU=
23+
github.com/sasha-s/go-deadlock v0.3.5/go.mod h1:bugP6EGbdGYObIlx7pUZtWqlvo8k9H6vCBBsiChJQ5U=
2024
github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
2125
github.com/stretchr/objx v0.4.0 h1:M2gUjqZET1qApGOWNSnZ49BAIMX4F/1plDv3+l31EJ4=
2226
github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw=

‎integration/go.mod‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,9 @@ require (
1212
github.com/Microsoft/go-winio v0.5.1 // indirect
1313
github.com/containernetworking/cni v1.2.2 // indirect
1414
github.com/davecgh/go-spew v1.1.1 // indirect
15+
github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7 // indirect
1516
github.com/pmezard/go-difflib v1.0.0 // indirect
17+
github.com/sasha-s/go-deadlock v0.3.5 // indirect
1618
github.com/sirupsen/logrus v1.7.0 // indirect
1719
golang.org/x/sync v0.0.0-20201207232520-09787c993a3a // indirect
1820
golang.org/x/sys v0.20.0 // indirect

‎integration/go.sum‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,8 @@ github.com/onsi/gomega v1.33.1/go.mod h1:U4R44UsT+9eLIaYRB2a5qajjtQYn0hauxvRm16A
8282
github.com/opencontainers/go-digest v1.0.0 h1:apOUWs51W5PlhuyGyz9FCeeBIOUDA/6nW8Oi/yOhh5U=
8383
github.com/opencontainers/go-digest v1.0.0/go.mod h1:0JzlMkj0TRzQZfJkVvzbP0HBR3IKzErnv2BNG4W4MAM=
8484
github.com/pelletier/go-toml v1.2.0/go.mod h1:5z9KED0ma1S8pY6P1sdut58dfprrGBbd/94hg7ilaic=
85+
github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7 h1:Dx7Ovyv/SFnMFw3fD4oEoeorXc6saIiQ23LrGLth0Gw=
86+
github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7/go.mod h1:pxMtw7cyUw6B2bRH0ZBANSPg+AoSud1I1iyJHI69jH4=
8587
github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
8688
github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
8789
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
@@ -97,6 +99,8 @@ github.com/prometheus/procfs v0.0.0-20190507164030-5867b95ac084/go.mod h1:TjEm7z
9799
github.com/prometheus/tsdb v0.7.1/go.mod h1:qhTCs0VvXwvX/y3TZrWD7rabWM+ijKTux40TwIPHuXU=
98100
github.com/rogpeppe/fastuuid v0.0.0-20150106093220-6724a57986af/go.mod h1:XWv6SoW27p1b0cqNHllgS5HIMJraePCO15w5zCzIWYg=
99101
github.com/russross/blackfriday/v2 v2.0.1/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
102+
github.com/sasha-s/go-deadlock v0.3.5 h1:tNCOEEDG6tBqrNDOX35j/7hL5FcFViG6awUGROb2NsU=
103+
github.com/sasha-s/go-deadlock v0.3.5/go.mod h1:bugP6EGbdGYObIlx7pUZtWqlvo8k9H6vCBBsiChJQ5U=
100104
github.com/shurcooL/sanitized_anchor_name v1.0.0/go.mod h1:1NzhyTcUVG4SuEtjjoZeVRXNmyL/1OwPU0+IJeTBvfc=
101105
github.com/sirupsen/logrus v1.2.0/go.mod h1:LxeOpSwHxABJmUn/MG1IvRgCAasNZTLOkJPxbbu5VWo=
102106
github.com/sirupsen/logrus v1.7.0 h1:ShrD1U9pZB12TX0cVy0DtePoCH97K8EtX+mg7ZARUtM=

‎mutex.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
//go:build !deadlocks && !race
2+
3+
/*
4+
Copyright The containerd Authors.
5+
6+
Licensed under the Apache License, Version 2.0 (the "License");
7+
you may not use this file except in compliance with the License.
8+
You may obtain a copy of the License at
9+
10+
http://www.apache.org/licenses/LICENSE-2.0
11+
12+
Unless required by applicable law or agreed to in writing, software
13+
distributed under the License is distributed on an "AS IS" BASIS,
14+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
See the License for the specific language governing permissions and
16+
limitations under the License.
17+
*/
18+
19+
package cni
20+
21+
import "sync"
22+
23+
type RWMutex = sync.RWMutex

‎mutex_deadlocks.go‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
//go:build deadlocks || race
2+
3+
/*
4+
Copyright The containerd Authors.
5+
6+
Licensed under the Apache License, Version 2.0 (the "License");
7+
you may not use this file except in compliance with the License.
8+
You may obtain a copy of the License at
9+
10+
http://www.apache.org/licenses/LICENSE-2.0
11+
12+
Unless required by applicable law or agreed to in writing, software
13+
distributed under the License is distributed on an "AS IS" BASIS,
14+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
See the License for the specific language governing permissions and
16+
limitations under the License.
17+
*/
18+
19+
package cni
20+
21+
import (
22+
"github.com/sasha-s/go-deadlock"
23+
)
24+
25+
type RWMutex = deadlock.RWMutex

‎result.go‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,6 @@ type Config struct {
6969
// interfaces created in the namespace. It returns an error if validation of
7070
// results fails, or if a network could not be found.
7171
func (c *libcni) createResult(results []*types100.Result) (*Result, error) {
72-
c.RLock()
73-
defer c.RUnlock()
7472
r := &Result{
7573
Interfaces: make(map[string]*Config),
7674
raw: results,

0 commit comments

Comments
 (0)