[PATCH] nwfilter_driver: Make reload fail if racing with stateCleanup()
Daniel P. Berrangé
berrange at redhat.com
Fri Apr 22 15:22:24 UTC 2022
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 at 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 :|
More information about the libvir-list
mailing list