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