Re: [libvirt] [PATCH] xen: fix race in refresh of config cache

The xenXMConfigCacheRefresh method scans /etc/xen and loads all config files it finds. It then scans its internal hash table and purges any (previously) loaded config files whose refresh timestamp does not match the timestamp recorded at the start of xenXMConfigCacheRefresh(). There is unfortunately a subtle flaw in this, because if loading the config files takes longer than 1 second, some of the config files will have a refresh timestamp that is 1 or more seconds different (newer) than is checked for. So we immediately purge a bunch of valid config files we just loaded. To avoid this flaw, we must pass the timestamp we record at the start of xenXMConfigCacheRefresh() into the xenXMConfigCacheAddFile() method, instead of letting the latter call time(NULL) again. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/xen/xen_inotify.c | 10 ++++++---- src/xen/xm_internal.c | 7 +++---- src/xen/xm_internal.h | 2 +- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/xen/xen_inotify.c b/src/xen/xen_inotify.c index cd1e2df..d81a35d 100644 --- a/src/xen/xen_inotify.c +++ b/src/xen/xen_inotify.c @@ -217,11 +217,11 @@ xenInotifyRemoveDomainConfigInfo(virConnectPtr conn, const char *fname) } static int -xenInotifyAddDomainConfigInfo(virConnectPtr conn, const char *fname) +xenInotifyAddDomainConfigInfo(virConnectPtr conn, const char *fname, time_t now) { xenUnifiedPrivatePtr priv = conn->privateData; return priv->useXenConfigCache ? - xenXMConfigCacheAddFile(conn, fname) : + xenXMConfigCacheAddFile(conn, fname, now) : xenInotifyXendDomainsDirAddEntry(conn, fname); } @@ -238,6 +238,7 @@ xenInotifyEvent(int watch ATTRIBUTE_UNUSED, char *tmp, *name; virConnectPtr conn = data; xenUnifiedPrivatePtr priv = NULL; + time_t now = time(NULL); VIR_DEBUG("got inotify event"); @@ -300,7 +301,7 @@ xenInotifyEvent(int watch ATTRIBUTE_UNUSED, } } else if (e->mask & (IN_CREATE | IN_CLOSE_WRITE | IN_MOVED_TO)) { virObjectEventPtr event; - if (xenInotifyAddDomainConfigInfo(conn, fname) < 0) { + if (xenInotifyAddDomainConfigInfo(conn, fname, now) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Error adding file to config cache")); goto cleanup; @@ -344,6 +345,7 @@ xenInotifyOpen(virConnectPtr conn, char *path; xenUnifiedPrivatePtr priv = conn->privateData; int direrr; + time_t now = time(NULL); virCheckFlags(VIR_CONNECT_RO, -1); @@ -374,7 +376,7 @@ xenInotifyOpen(virConnectPtr conn, return -1; } - if (xenInotifyAddDomainConfigInfo(conn, path) < 0) { + if (xenInotifyAddDomainConfigInfo(conn, path, now) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Error adding file to config list")); closedir(dh); diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 59b1cd4..07b9eb4 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -191,15 +191,14 @@ xenXMConfigCacheRemoveFile(virConnectPtr conn, const char *filename) * calling this function */ int -xenXMConfigCacheAddFile(virConnectPtr conn, const char *filename) +xenXMConfigCacheAddFile(virConnectPtr conn, const char *filename, time_t now) { xenUnifiedPrivatePtr priv = conn->privateData; xenXMConfCachePtr entry; struct stat st; int newborn = 0; - time_t now = time(NULL); - VIR_DEBUG("Adding file %s", filename); + VIR_DEBUG("Adding file %s %lld", filename, (long long)now); /* Get modified time */ if ((stat(filename, &st) < 0)) { @@ -373,7 +372,7 @@ xenXMConfigCacheRefresh(virConnectPtr conn) /* If we already have a matching entry and it is not modified, then carry on to next one*/ - if (xenXMConfigCacheAddFile(conn, path) < 0) { + if (xenXMConfigCacheAddFile(conn, path, now) < 0) { /* Ignoring errors, since a lot of stuff goes wrong in /etc/xen */ } diff --git a/src/xen/xm_internal.h b/src/xen/xm_internal.h index 25b4fd5..0246895 100644 --- a/src/xen/xm_internal.h +++ b/src/xen/xm_internal.h @@ -31,7 +31,7 @@ # include "domain_conf.h" int xenXMConfigCacheRefresh (virConnectPtr conn); -int xenXMConfigCacheAddFile(virConnectPtr conn, const char *filename); +int xenXMConfigCacheAddFile(virConnectPtr conn, const char *filename, time_t now); int xenXMConfigCacheRemoveFile(virConnectPtr conn, const char *filename); int xenXMOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags);