fix: recursive RLock() mutex acquision

smira · smira · commit 75a24409e819 · 2024-12-24T16:19:44.000+04:00
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 <andrey.smirnov@siderolabs.com>
diff --git a/cni.go b/cni.go
@@ -80,7 +80,10 @@ type libcni struct {
 	cniConfig    cnilibrary.CNI
 	networkCount int // minimum network plugin configurations needed to initialize cni
 	networks     []*Network
-	sync.RWMutex
+	// Mutex contract:
+	// - lock in public methods: write lock when mutating the state, read lock when reading the state.
+	// - never lock in private methods.
+	RWMutex
 }
 
 func defaultCNIConfig() *libcni {
@@ -135,11 +138,11 @@ func (c *libcni) Load(opts ...Opt) error {
 
 // Status returns the status of CNI initialization.
 func (c *libcni) Status() error {
+	c.RLock()
+	defer c.RUnlock()
 	if err := c.ready(); err != nil {
 		return err
 	}
-	c.RLock()
-	defer c.RUnlock()
 	// STATUS is only called for CNI Version 1.1.0 or greater. It is ignored for previous versions.
 	for _, v := range c.networks {
 		err := c.cniConfig.GetStatusNetworkList(context.Background(), v.config)
@@ -162,11 +165,11 @@ func (c *libcni) Networks() []*Network {
 
 // Setup setups the network in the namespace and returns a Result
 func (c *libcni) Setup(ctx context.Context, id string, path string, opts ...NamespaceOpts) (*Result, error) {
+	c.RLock()
+	defer c.RUnlock()
 	if err := c.ready(); err != nil {
 		return nil, err
 	}
-	c.RLock()
-	defer c.RUnlock()
 	ns, err := newNamespace(id, path, opts...)
 	if err != nil {
 		return nil, err
@@ -180,11 +183,11 @@ func (c *libcni) Setup(ctx context.Context, id string, path string, opts ...Name
 
 // SetupSerially setups the network in the namespace and returns a Result
 func (c *libcni) SetupSerially(ctx context.Context, id string, path string, opts ...NamespaceOpts) (*Result, error) {
+	c.RLock()
+	defer c.RUnlock()
 	if err := c.ready(); err != nil {
 		return nil, err
 	}
-	c.RLock()
-	defer c.RUnlock()
 	ns, err := newNamespace(id, path, opts...)
 	if err != nil {
 		return nil, err
@@ -198,7 +201,7 @@ func (c *libcni) SetupSerially(ctx context.Context, id string, path string, opts
 
 func (c *libcni) attachNetworksSerially(ctx context.Context, ns *Namespace) ([]*types100.Result, error) {
 	var results []*types100.Result
-	for _, network := range c.Networks() {
+	for _, network := range c.networks {
 		r, err := network.Attach(ctx, ns)
 		if err != nil {
 			return nil, err
@@ -223,15 +226,15 @@ func asynchAttach(ctx context.Context, index int, n *Network, ns *Namespace, wg
 func (c *libcni) attachNetworks(ctx context.Context, ns *Namespace) ([]*types100.Result, error) {
 	var wg sync.WaitGroup
 	var firstError error
-	results := make([]*types100.Result, len(c.Networks()))
+	results := make([]*types100.Result, len(c.networks))
 	rc := make(chan asynchAttachResult)
 
-	for i, network := range c.Networks() {
+	for i, network := range c.networks {
 		wg.Add(1)
 		go asynchAttach(ctx, i, network, ns, &wg, rc)
 	}
 
-	for range c.Networks() {
+	for range c.networks {
 		rs := <-rc
 		if rs.err != nil && firstError == nil {
 			firstError = rs.err
@@ -245,16 +248,16 @@ func (c *libcni) attachNetworks(ctx context.Context, ns *Namespace) ([]*types100
 
 // Remove removes the network config from the namespace
 func (c *libcni) Remove(ctx context.Context, id string, path string, opts ...NamespaceOpts) error {
+	c.RLock()
+	defer c.RUnlock()
 	if err := c.ready(); err != nil {
 		return err
 	}
-	c.RLock()
-	defer c.RUnlock()
 	ns, err := newNamespace(id, path, opts...)
 	if err != nil {
 		return err
 	}
-	for _, network := range c.Networks() {
+	for _, network := range c.networks {
 		if err := network.Remove(ctx, ns); err != nil {
 			// Based on CNI spec v0.7.0, empty network namespace is allowed to
 			// 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
 
 // Check checks if the network is still in desired state
 func (c *libcni) Check(ctx context.Context, id string, path string, opts ...NamespaceOpts) error {
+	c.RLock()
+	defer c.RUnlock()
 	if err := c.ready(); err != nil {
 		return err
 	}
-	c.RLock()
-	defer c.RUnlock()
 	ns, err := newNamespace(id, path, opts...)
 	if err != nil {
 		return err
 	}
-	for _, network := range c.Networks() {
+	for _, network := range c.networks {
 		err := network.Check(ctx, ns)
 		if err != nil {
 			return err
@@ -329,8 +332,6 @@ func (c *libcni) reset() {
 }
 
 func (c *libcni) ready() error {
-	c.RLock()
-	defer c.RUnlock()
 	if len(c.networks) < c.networkCount {
 		return ErrCNINotInitialized
 	}
diff --git a/cni_test.go b/cni_test.go
@@ -35,12 +35,13 @@ import (
 // TestLibCNIType020 tests the cni version 0.2.0 plugin
 // config and parses the result into structured data
 func TestLibCNIType020(t *testing.T) {
+	t.Parallel()
+
 	// Get the default CNI config
 	l := defaultCNIConfig()
 
 	// Create a fake cni config directory and file
-	cniDir, confDir := makeFakeCNIConfig(t)
-	defer tearDownCNIConfig(t, cniDir)
+	_, confDir := makeFakeCNIConfig(t)
 	l.pluginConfDir = confDir
 	// Set the minimum network count as 2 for this test
 	l.networkCount = 2
@@ -112,11 +113,12 @@ func TestLibCNIType020(t *testing.T) {
 // TestLibCNIType040 tests the cni version 0.4.0 plugin
 // config and parses the result into structured data
 func TestLibCNIType040(t *testing.T) {
+	t.Parallel()
+
 	// Get the default CNI config
 	l := defaultCNIConfig()
 	// Create a fake cni config directory and file
-	cniDir, confDir := makeFakeCNIConfig(t)
-	defer tearDownCNIConfig(t, cniDir)
+	_, confDir := makeFakeCNIConfig(t)
 	l.pluginConfDir = confDir
 	// Set the minimum network count as 2 for this test
 	l.networkCount = 2
@@ -200,11 +202,12 @@ func TestLibCNIType040(t *testing.T) {
 // TestLibCNIType100 tests the cni version 1.0.0 plugin
 // config and parses the result into structured data
 func TestLibCNIType100(t *testing.T) {
+	t.Parallel()
+
 	// Get the default CNI config
 	l := defaultCNIConfig()
 	// Create a fake cni config directory and file
-	cniDir, confDir := makeFakeCNIConfig(t)
-	defer tearDownCNIConfig(t, cniDir)
+	_, confDir := makeFakeCNIConfig(t)
 	l.pluginConfDir = confDir
 	// Set the minimum network count as 2 for this test
 	l.networkCount = 2
@@ -290,11 +293,12 @@ func TestLibCNIType100(t *testing.T) {
 // TestLibCNIType120 tests the cni version 1.1.0 plugin
 // config and parses the result into structured data
 func TestLibCNIType120(t *testing.T) {
+	t.Parallel()
+
 	// Get the default CNI config
 	l := defaultCNIConfig()
 	// Create a fake cni config directory and file
-	cniDir, confDir := buildFakeConfig(t)
-	defer tearDownCNIConfig(t, cniDir)
+	_, confDir := buildFakeConfig(t)
 	l.pluginConfDir = confDir
 	// Set the minimum network count as 2 for this test
 	l.networkCount = 2
@@ -382,11 +386,12 @@ func TestLibCNIType120(t *testing.T) {
 }
 
 func TestLibCNIType120FailStatus(t *testing.T) {
+	t.Parallel()
+
 	// Get the default CNI config
 	l := defaultCNIConfig()
 	// Create a fake cni config directory and file
-	cniDir, confDir := buildFakeConfig(t)
-	defer tearDownCNIConfig(t, cniDir)
+	_, confDir := buildFakeConfig(t)
 	l.pluginConfDir = confDir
 	// Set the minimum network count as 2 for this test
 	l.networkCount = 2
diff --git a/deprecated.go b/deprecated.go
@@ -30,5 +30,7 @@ type CNIResult = Result //revive:disable // type name will be used as cni.CNIRes
 // results fails, or if a network could not be found.
 // Deprecated: do not use
 func (c *libcni) GetCNIResultFromResults(results []*types100.Result) (*Result, error) {
+	c.RLock()
+	defer c.RUnlock()
 	return c.createResult(results)
 }
diff --git a/go.mod b/go.mod
@@ -4,11 +4,13 @@ go 1.21
 
 require (
 	github.com/containernetworking/cni v1.2.2
+	github.com/sasha-s/go-deadlock v0.3.5
 	github.com/stretchr/testify v1.8.0
 )
 
 require (
 	github.com/davecgh/go-spew v1.1.1 // indirect
+	github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/stretchr/objx v0.4.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
diff --git a/go.sum b/go.sum
@@ -15,8 +15,12 @@ github.com/onsi/ginkgo/v2 v2.19.0 h1:9Cnnf7UHo57Hy3k6/m5k3dRfGTMXGvxhHFvkDTCTpvA
 github.com/onsi/ginkgo/v2 v2.19.0/go.mod h1:rlwLi9PilAFJ8jCg9UE1QP6VBpd6/xj3SRC0d6TU0To=
 github.com/onsi/gomega v1.33.1 h1:dsYjIxxSR755MDmKVsaFQTE22ChNBcuuTWgkUDSubOk=
 github.com/onsi/gomega v1.33.1/go.mod h1:U4R44UsT+9eLIaYRB2a5qajjtQYn0hauxvRm16AVYg0=
+github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7 h1:Dx7Ovyv/SFnMFw3fD4oEoeorXc6saIiQ23LrGLth0Gw=
+github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7/go.mod h1:pxMtw7cyUw6B2bRH0ZBANSPg+AoSud1I1iyJHI69jH4=
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
+github.com/sasha-s/go-deadlock v0.3.5 h1:tNCOEEDG6tBqrNDOX35j/7hL5FcFViG6awUGROb2NsU=
+github.com/sasha-s/go-deadlock v0.3.5/go.mod h1:bugP6EGbdGYObIlx7pUZtWqlvo8k9H6vCBBsiChJQ5U=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/objx v0.4.0 h1:M2gUjqZET1qApGOWNSnZ49BAIMX4F/1plDv3+l31EJ4=
 github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw=
diff --git a/integration/go.mod b/integration/go.mod
@@ -12,7 +12,9 @@ require (
 	github.com/Microsoft/go-winio v0.5.1 // indirect
 	github.com/containernetworking/cni v1.2.2 // indirect
 	github.com/davecgh/go-spew v1.1.1 // indirect
+	github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
+	github.com/sasha-s/go-deadlock v0.3.5 // indirect
 	github.com/sirupsen/logrus v1.7.0 // indirect
 	golang.org/x/sync v0.0.0-20201207232520-09787c993a3a // indirect
 	golang.org/x/sys v0.20.0 // indirect
diff --git a/integration/go.sum b/integration/go.sum
@@ -82,6 +82,8 @@ github.com/onsi/gomega v1.33.1/go.mod h1:U4R44UsT+9eLIaYRB2a5qajjtQYn0hauxvRm16A
 github.com/opencontainers/go-digest v1.0.0 h1:apOUWs51W5PlhuyGyz9FCeeBIOUDA/6nW8Oi/yOhh5U=
 github.com/opencontainers/go-digest v1.0.0/go.mod h1:0JzlMkj0TRzQZfJkVvzbP0HBR3IKzErnv2BNG4W4MAM=
 github.com/pelletier/go-toml v1.2.0/go.mod h1:5z9KED0ma1S8pY6P1sdut58dfprrGBbd/94hg7ilaic=
+github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7 h1:Dx7Ovyv/SFnMFw3fD4oEoeorXc6saIiQ23LrGLth0Gw=
+github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7/go.mod h1:pxMtw7cyUw6B2bRH0ZBANSPg+AoSud1I1iyJHI69jH4=
 github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
 github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
 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
 github.com/prometheus/tsdb v0.7.1/go.mod h1:qhTCs0VvXwvX/y3TZrWD7rabWM+ijKTux40TwIPHuXU=
 github.com/rogpeppe/fastuuid v0.0.0-20150106093220-6724a57986af/go.mod h1:XWv6SoW27p1b0cqNHllgS5HIMJraePCO15w5zCzIWYg=
 github.com/russross/blackfriday/v2 v2.0.1/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
+github.com/sasha-s/go-deadlock v0.3.5 h1:tNCOEEDG6tBqrNDOX35j/7hL5FcFViG6awUGROb2NsU=
+github.com/sasha-s/go-deadlock v0.3.5/go.mod h1:bugP6EGbdGYObIlx7pUZtWqlvo8k9H6vCBBsiChJQ5U=
 github.com/shurcooL/sanitized_anchor_name v1.0.0/go.mod h1:1NzhyTcUVG4SuEtjjoZeVRXNmyL/1OwPU0+IJeTBvfc=
 github.com/sirupsen/logrus v1.2.0/go.mod h1:LxeOpSwHxABJmUn/MG1IvRgCAasNZTLOkJPxbbu5VWo=
 github.com/sirupsen/logrus v1.7.0 h1:ShrD1U9pZB12TX0cVy0DtePoCH97K8EtX+mg7ZARUtM=
diff --git a/mutex.go b/mutex.go
@@ -0,0 +1,23 @@
+//go:build !deadlocks && !race
+
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       https://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package cni
+
+import "sync"
+
+type RWMutex = sync.RWMutex
diff --git a/mutex_deadlocks.go b/mutex_deadlocks.go
@@ -0,0 +1,25 @@
+//go:build deadlocks || race
+
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       https://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package cni
+
+import (
+	"github.com/sasha-s/go-deadlock"
+)
+
+type RWMutex = deadlock.RWMutex
diff --git a/result.go b/result.go
@@ -69,8 +69,6 @@ type Config struct {
 // interfaces created in the namespace. It returns an error if validation of
 // results fails, or if a network could not be found.
 func (c *libcni) createResult(results []*types100.Result) (*Result, error) {
-	c.RLock()
-	defer c.RUnlock()
 	r := &Result{
 		Interfaces: make(map[string]*Config),
 		raw:        results,
diff --git a/testutils.go b/testutils.go
@@ -23,22 +23,11 @@ import (
 	"testing"
 )
 
-func makeTmpDir(prefix string) (string, error) {
-	tmpDir, err := os.MkdirTemp("", prefix)
-	if err != nil {
-		return "", err
-	}
-	return tmpDir, nil
-}
-
 func makeFakeCNIConfig(t *testing.T) (string, string) {
-	cniDir, err := makeTmpDir("fakecni")
-	if err != nil {
-		t.Fatalf("Failed to create plugin config dir: %v", err)
-	}
+	cniDir := t.TempDir()
 
 	cniConfDir := path.Join(cniDir, "net.d")
-	err = os.MkdirAll(cniConfDir, 0777)
+	err := os.MkdirAll(cniConfDir, 0777)
 	if err != nil {
 		t.Fatalf("Failed to create network config dir: %v", err)
 	}
@@ -69,13 +58,6 @@ func makeFakeCNIConfig(t *testing.T) (string, string) {
 	return cniDir, cniConfDir
 }
 
-func tearDownCNIConfig(t *testing.T, confDir string) {
-	err := os.RemoveAll(confDir)
-	if err != nil {
-		t.Fatalf("Failed to cleanup CNI configs: %v", err)
-	}
-}
-
 func buildFakeConfig(t *testing.T) (string, string) {
 	conf := `
 	{
@@ -111,13 +93,10 @@ func buildFakeConfig(t *testing.T) (string, string) {
 	]
 	}`
 
-	cniDir, err := makeTmpDir("fakecni")
-	if err != nil {
-		t.Fatalf("Failed to create plugin config dir: %v", err)
-	}
+	cniDir := t.TempDir()
 
 	cniConfDir := path.Join(cniDir, "net.d")
-	err = os.MkdirAll(cniConfDir, 0777)
+	err := os.MkdirAll(cniConfDir, 0777)
 	if err != nil {
 		t.Fatalf("Failed to create network config dir: %v", err)
 	}

Original file line number	Diff line number	Diff line change
`@@ -30,5 +30,7 @@ type CNIResult = Result //revive:disable // type name will be used as cni.CNIRes`
`30`	`30`	`// results fails, or if a network could not be found.`
`31`	`31`	`// Deprecated: do not use`
`32`	`32`	`func (c libcni) GetCNIResultFromResults(results []types100.Result) (*Result, error) {`
	`33`	`+ c.RLock()`
	`34`	`+ defer c.RUnlock()`
`33`	`35`	`return c.createResult(results)`
`34`	`36`	`}`