When one thread is trying to reload NWFilter driver (by running
nwfilterStateReload()) but there's another thread that's
concurrently running nwfilterStateCleanup() a crash may occur.
This is despite nwfilterStateReload() checking for driver !=
NULL, because is done so without @driverMutex held. A typical
TOCTOU. Fortunately, the mutex is always initialized, so the
mutex can be acquired at all times and @driver can be checked
with the lock held.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2075837
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/nwfilter/nwfilter_driver.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index b66ba22737..d028efafbe 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -309,6 +309,8 @@ nwfilterStateInitialize(bool privileged,
 static int
 nwfilterStateReload(void)
 {
+    VIR_LOCK_GUARD lock = virLockGuardLock(&driverMutex);
+
     if (!driver)
         return -1;
 
@@ -319,15 +321,13 @@ nwfilterStateReload(void)
     /* shut down all threads -- they will be restarted if necessary */
     virNWFilterLearnThreadsTerminate(true);
 
-    VIR_WITH_MUTEX_LOCK_GUARD(&driverMutex) {
-        VIR_WITH_MUTEX_LOCK_GUARD(&driver->updateLock) {
-            virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir);
-        }
-
-
-        virNWFilterBuildAll(driver, false);
+    VIR_WITH_MUTEX_LOCK_GUARD(&driver->updateLock) {
+        virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir);
     }
 
+
+    virNWFilterBuildAll(driver, false);
+
     return 0;
 }
 
-- 
2.35.1

On Fri, Apr 22, 2022 at 04:45:37PM +0200, Michal Privoznik wrote:
> When one thread is trying to reload NWFilter driver (by running
> nwfilterStateReload()) but there's another thread that's
> concurrently running nwfilterStateCleanup() a crash may occur.
> This is despite nwfilterStateReload() checking for driver !=
> NULL, because is done so without @driverMutex held. A typical
> TOCTOU. Fortunately, the mutex is always initialized, so the
> mutex can be acquired at all times and @driver can be checked
> with the lock held.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2075837
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/nwfilter/nwfilter_driver.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)

I'm concerned that we might have latent bugs in other
drivers too, and/or be at risk of introducing them
later.

I've always thought of StateReload as been non-overlapping
with StateCleanup, though I realize now that's not actually
the case in practice.

I wonder if it would better if we made remote_daemon.c
avoiding calling StateCleanup, until any StateReload
has finished, to eliminate the entire class of problem
across all the drivers.

> 
> diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
> index b66ba22737..d028efafbe 100644
> --- a/src/nwfilter/nwfilter_driver.c
> +++ b/src/nwfilter/nwfilter_driver.c
> @@ -309,6 +309,8 @@ nwfilterStateInitialize(bool privileged,
>  static int
>  nwfilterStateReload(void)
>  {
> +    VIR_LOCK_GUARD lock = virLockGuardLock(&driverMutex);
> +
>      if (!driver)
>          return -1;
>  
> @@ -319,15 +321,13 @@ nwfilterStateReload(void)
>      /* shut down all threads -- they will be restarted if necessary */
>      virNWFilterLearnThreadsTerminate(true);
>  
> -    VIR_WITH_MUTEX_LOCK_GUARD(&driverMutex) {
> -        VIR_WITH_MUTEX_LOCK_GUARD(&driver->updateLock) {
> -            virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir);
> -        }
> -
> -
> -        virNWFilterBuildAll(driver, false);
> +    VIR_WITH_MUTEX_LOCK_GUARD(&driver->updateLock) {
> +        virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir);
>      }
>  
> +
> +    virNWFilterBuildAll(driver, false);
> +
>      return 0;
>  }
>  
> -- 
> 2.35.1
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|