Skip to content

Commit

Permalink
Fix race condition in GetCurrentNS
Browse files Browse the repository at this point in the history
In GetCurrentNS, If there is a context-switch between
getCurrentThreadNetNSPath and GetNS, another goroutine may execute in
the original thread and change its network namespace, then the original
goroutine would get the updated network namespace, which could lead to
unexpected behavior, especially when GetCurrentNS is used to get the
host network namespace in netNS.Do.

The added test has a chance to reproduce it with "-count=50".

The patch fixes it by locking the thread in GetCurrentNS.
  • Loading branch information
tnqn committed Aug 21, 2020
1 parent bd58999 commit 1045747
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 0 deletions.
5 changes: 5 additions & 0 deletions pkg/ns/ns_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ import (

// Returns an object representing the current OS thread's network namespace
func GetCurrentNS() (NetNS, error) {
// Lock the thread in case other goroutine executes in it and changes its
// network namespace after getCurrentThreadNetNSPath(), otherwise it might
// return an unexpected network namespace.
runtime.LockOSThread()
defer runtime.UnlockOSThread()
return GetNS(getCurrentThreadNetNSPath())
}

Expand Down
28 changes: 28 additions & 0 deletions pkg/ns/ns_linux_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"io/ioutil"
"os"
"path/filepath"
"sync"

"github.com/containernetworking/plugins/pkg/ns"
"github.com/containernetworking/plugins/pkg/testutils"
Expand Down Expand Up @@ -118,6 +119,33 @@ var _ = Describe("Linux namespace operations", func() {
Expect(err).NotTo(HaveOccurred())
})

Context("when called concurrently", func() {
It("provides the original namespace as the argument to the callback", func() {
concurrency := 200
origNS, err := ns.GetCurrentNS()
Expect(err).NotTo(HaveOccurred())
origNSInode, err := getInodeNS(origNS)
Expect(err).NotTo(HaveOccurred())

var wg sync.WaitGroup
wg.Add(concurrency)
for i := 0; i < concurrency; i++ {
go func() {
defer wg.Done()
targetNetNS.Do(func(hostNS ns.NetNS) error {
defer GinkgoRecover()

hostNSInode, err := getInodeNS(hostNS)
Expect(err).NotTo(HaveOccurred())
Expect(hostNSInode).To(Equal(origNSInode))
return nil
})
}()
}
wg.Wait()
})
})

Context("when the callback returns an error", func() {
It("restores the calling thread to the original namespace before returning", func() {
err := originalNetNS.Do(func(ns.NetNS) error {
Expand Down

0 comments on commit 1045747

Please sign in to comment.