From af6f6091e02bb46633666ce30d4c6533a52688a5 Mon Sep 17 00:00:00 2001
From: Michal Privoznik <mprivozn@redhat.com>
Date: Fri, 18 Mar 2022 17:14:54 +0100
Subject: [PATCH] virNWFilterLockIface: Preserve correct lock ordering

In the not so distant past, the lock ordering in
virNWFilterLockIface() was as follows: global mutex ifaceMapLock
was acquired, then internal representation of given interface was
looked up in a hash table (or created brand new if none was
found), the global lock was released and the lock of the
interface was acquired.

But this was mistakenly changed as the function was rewritten to
use automatic mutexes, because now the global lock is held
throughout the whole run of the function and thus the interface
specific lock is acquired with the global lock held. This results
in a deadlock.

Fixes: dd8150c48dcf94e8d3b0481be08eeef822b98b02
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
---
 src/nwfilter/nwfilter_learnipaddr.c | 53 +++++++++++++++--------------
 1 file changed, 28 insertions(+), 25 deletions(-)

diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c
index 2c85972012..ec2d337188 100644
--- a/src/nwfilter/nwfilter_learnipaddr.c
+++ b/src/nwfilter/nwfilter_learnipaddr.c
@@ -143,37 +143,40 @@ static bool threadsTerminate;
 int
 virNWFilterLockIface(const char *ifname)
 {
-    VIR_LOCK_GUARD lock = virLockGuardLock(&ifaceMapLock);
-    virNWFilterIfaceLock *ifaceLock = virHashLookup(ifaceLockMap, ifname);
+    virNWFilterIfaceLock *ifaceLock = NULL;
 
-    if (!ifaceLock) {
-        ifaceLock = g_new0(virNWFilterIfaceLock, 1);
+    VIR_WITH_MUTEX_LOCK_GUARD(&ifaceMapLock) {
+        ifaceLock = virHashLookup(ifaceLockMap, ifname);
 
-        if (virMutexInitRecursive(&ifaceLock->lock) < 0) {
-            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                           _("mutex initialization failed"));
-            g_free(ifaceLock);
-            return -1;
+        if (!ifaceLock) {
+            ifaceLock = g_new0(virNWFilterIfaceLock, 1);
+
+            if (virMutexInitRecursive(&ifaceLock->lock) < 0) {
+                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                               _("mutex initialization failed"));
+                g_free(ifaceLock);
+                return -1;
+            }
+
+            if (virStrcpyStatic(ifaceLock->ifname, ifname) < 0) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("interface name %s does not fit into buffer"),
+                               ifaceLock->ifname);
+                g_free(ifaceLock);
+                return -1;
+            }
+
+            while (virHashAddEntry(ifaceLockMap, ifname, ifaceLock)) {
+                g_free(ifaceLock);
+                return -1;
+            }
+
+            ifaceLock->refctr = 0;
         }
 
-        if (virStrcpyStatic(ifaceLock->ifname, ifname) < 0) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("interface name %s does not fit into buffer"),
-                           ifaceLock->ifname);
-            g_free(ifaceLock);
-            return -1;
-        }
-
-        while (virHashAddEntry(ifaceLockMap, ifname, ifaceLock)) {
-            g_free(ifaceLock);
-            return -1;
-        }
-
-        ifaceLock->refctr = 0;
+        ifaceLock->refctr++;
     }
 
-    ifaceLock->refctr++;
-
     virMutexLock(&ifaceLock->lock);
 
     return 0;