From 7ad1bf35f958d944404e06d7564d29e252405e61 Mon Sep 17 00:00:00 2001
From: Dan Williams <dcbw@redhat.com>
Date: Fri, 15 Nov 2019 14:26:47 -0600
Subject: [PATCH] ovn/namespace: fix some namespace locking issues

First, updateNamespace() didn't have locking for oc.namespaceMutex
Introduced by c3def15ad98628 (PR #885).

Second, getNamespaceLock() didn't protect against concurrent
access of oc.namespaceMutex when it checked it the second time.
That's fishy anyway; we serialize Namespace events from the
watch factory and getNamespaceLock() is only called from event
handler functions, so it doesn't seem possible for the namespace
to be deleted between grabbing these two locks.

Signed-off-by: Dan Williams <dcbw@redhat.com>
---
 go-controller/pkg/ovn/namespace.go | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/go-controller/pkg/ovn/namespace.go b/go-controller/pkg/ovn/namespace.go
index a68599ebd72..77a004d82b8 100644
--- a/go-controller/pkg/ovn/namespace.go
+++ b/go-controller/pkg/ovn/namespace.go
@@ -161,14 +161,13 @@ func (oc *Controller) multicastDeleteNamespace(ns *kapi.Namespace) {
 func (oc *Controller) AddNamespace(ns *kapi.Namespace) {
 	logrus.Debugf("Adding namespace: %s", ns.Name)
 	oc.namespaceMutexMutex.Lock()
-	if oc.namespaceMutex[ns.Name] == nil {
-		oc.namespaceMutex[ns.Name] = &sync.Mutex{}
+	mutex, ok := oc.namespaceMutex[ns.Name]
+	if !ok {
+		mutex = &sync.Mutex{}
+		oc.namespaceMutex[ns.Name] = mutex
 	}
-
-	// A big fat lock per namespace to prevent race conditions
-	// with namespace resources like address sets and deny acls.
-	oc.namespaceMutex[ns.Name].Lock()
-	defer oc.namespaceMutex[ns.Name].Unlock()
+	mutex.Lock()
+	defer mutex.Unlock()
 	oc.namespaceMutexMutex.Unlock()
 
 	oc.namespaceAddressSet[ns.Name] = make(map[string]string)
@@ -204,10 +203,11 @@ func (oc *Controller) AddNamespace(ns *kapi.Namespace) {
 func (oc *Controller) updateNamespace(old, newer *kapi.Namespace) {
 	logrus.Debugf("Updating namespace: old %s new %s", old.Name, newer.Name)
 
-	// A big fat lock per namespace to prevent race conditions
-	// with namespace resources like address sets and deny acls.
-	oc.namespaceMutex[newer.Name].Lock()
-	defer oc.namespaceMutex[newer.Name].Unlock()
+	mutex := oc.getNamespaceLock(newer.Name)
+	if mutex == nil {
+		return
+	}
+	defer mutex.Unlock()
 
 	oc.multicastUpdateNamespace(newer)
 }
@@ -244,12 +244,5 @@ func (oc *Controller) getNamespaceLock(ns string) *sync.Mutex {
 
 	// lock the individual namespace
 	mutex.Lock()
-
-	// check that the namespace wasn't deleted between getting the two locks
-	if _, ok := oc.namespaceMutex[ns]; !ok {
-		mutex.Unlock()
-		return nil
-	}
-
 	return mutex
 }