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

[Richard W.M. Jones]

On Fri, Sep 11, 2015 at 02:41:02PM +0100, Daniel P. Berrange wrote:
> 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 at redhat.com>
> ---
>  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);

Good one :-)

ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/