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 :|