[PATCH] libxl: fix issues in 38cd0664

[PATCH] libxl: fix issues in 38cd0664

[Wei Liu]

A few issues were introduced in 38cd0664 ("libxl/arm: Add the size of
ACPI tables to maxmem"):

1. d_config was not properly initialised and disposed of.
2. using libxl_retrieve_domain_configuration caused thread to
   deadlock itself.

Fix those issues by:

1. properly initialise and dispose of d_config.
2. switch to use libxl__get_domain_configuration.

Note that in theory we can refactor libxl_retrieve_domain_configuration
a bit to get a function without locking, but up until the calculation of
extra memory only relies on static configuration, hence we use the
stored configuration only.

Reported-by: Anthony PERARD <anthony.perard@citrix.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Tested-by: Anthony PERARD <anthony.perard@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Shannon Zhao <shannon.zhao@linaro.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 432e5d9..33c5e4c 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4029,6 +4029,8 @@ int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint64_t max_memkb)
     libxl__domain_userdata_lock *lock = NULL;
     libxl_domain_config d_config;
 
+    libxl_domain_config_init(&d_config);
+
     CTX_LOCK;
 
     lock = libxl__lock_domain_userdata(gc, domid);
@@ -4054,7 +4056,7 @@ int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint64_t max_memkb)
         goto out;
     }
 
-    rc = libxl_retrieve_domain_configuration(ctx, domid, &d_config);
+    rc = libxl__get_domain_configuration(gc, domid, &d_config);
     if (rc < 0) {
         LOGE(ERROR, "unable to retrieve domain configuration");
         goto out;
@@ -4076,6 +4078,7 @@ int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint64_t max_memkb)
 
     rc = 0;
 out:
+    libxl_domain_config_dispose(&d_config);
     if (lock) libxl__unlock_domain_userdata(lock);
     CTX_UNLOCK;
     GC_FREE;
@@ -4177,6 +4180,8 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
     libxl__domain_userdata_lock *lock;
     libxl_domain_config d_config;
 
+    libxl_domain_config_init(&d_config);
+
     CTX_LOCK;
 
     lock = libxl__lock_domain_userdata(gc, domid);
@@ -4185,7 +4190,7 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
         goto out_no_transaction;
     }
 
-    rc = libxl_retrieve_domain_configuration(ctx, domid, &d_config);
+    rc = libxl__get_domain_configuration(gc, domid, &d_config);
     if (rc < 0) {
         LOGE(ERROR, "unable to retrieve domain configuration");
         goto out_no_transaction;
@@ -4318,6 +4323,7 @@ out:
             goto retry_transaction;
 
 out_no_transaction:
+    libxl_domain_config_dispose(&d_config);
     if (lock) libxl__unlock_domain_userdata(lock);
     CTX_UNLOCK;
     GC_FREE;
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] libxl: fix issues in 38cd0664

[Ian Jackson]

Wei Liu writes ("[PATCH] libxl: fix issues in 38cd0664"):
> A few issues were introduced in 38cd0664 ("libxl/arm: Add the size of
> ACPI tables to maxmem"):
> 
> 1. d_config was not properly initialised and disposed of.
> 2. using libxl_retrieve_domain_configuration caused thread to
>    deadlock itself.
> 
> Fix those issues by:
> 
> 1. properly initialise and dispose of d_config.
> 2. switch to use libxl__get_domain_configuration.
> 
> Note that in theory we can refactor libxl_retrieve_domain_configuration
> a bit to get a function without locking, but up until the calculation of
> extra memory only relies on static configuration, hence we use the
> stored configuration only.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

I think we should probably do the patch below, too.  I've checked that
it doesn't break the build (after your patch) :-).

Thanks,
Ian.

From abe530fb827edb22ed3db51d56f07b88cf8f0e17 Mon Sep 17 00:00:00 2001
From: Ian Jackson <ian.jackson@eu.citrix.com>
Date: Tue, 4 Oct 2016 10:19:36 +0100
Subject: [PATCH] libxl: Mark libxl_retrieve_domain_configuration as for
 external callers only

This function takes the userdata lock.  Incautious use inside libxl
can result in nested acquisition of that lock, and deadlock.

There is no good reason to use this function inside libxl, but it is a
superficially attractive option.  Make future regressions easier to
spot by marking the function for external use only.

Similar arguments apply for the application-facing userdata accessors,
so do those too.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 969a089..acbf476 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1352,7 +1352,8 @@ void libxl_domain_config_dispose(libxl_domain_config *d_config);
  * works with DomU.
  */
 int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
-                                        libxl_domain_config *d_config);
+                                        libxl_domain_config *d_config)
+                                        LIBXL_EXTERNAL_CALLERS_ONLY;
 
 int libxl_domain_suspend(libxl_ctx *ctx, uint32_t domid, int fd,
                          int flags, /* LIBXL_SUSPEND_* */
@@ -1945,12 +1946,14 @@ void libxl_cpuid_set(libxl_ctx *ctx, uint32_t domid,
  */
 int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
                               const char *userdata_userid,
-                              const uint8_t *data, int datalen);
+                              const uint8_t *data, int datalen)
+                              LIBXL_EXTERNAL_CALLERS_ONLY;
   /* If datalen==0, data is not used and the user data for
    * that domain and userdata_userid is deleted. */
 int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid,
                                  const char *userdata_userid,
-                                 uint8_t **data_r, int *datalen_r);
+                                 uint8_t **data_r, int *datalen_r)
+                                 LIBXL_EXTERNAL_CALLERS_ONLY;
   /* On successful return, *data_r is from malloc.
    * If there is no data for that domain and userdata_userid,
    * *data_r and *datalen_r will be set to 0.
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] libxl: fix issues in 38cd0664

[Wei Liu]

On Tue, Oct 04, 2016 at 10:48:57AM +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH] libxl: fix issues in 38cd0664"):
> > A few issues were introduced in 38cd0664 ("libxl/arm: Add the size of
> > ACPI tables to maxmem"):
> > 
> > 1. d_config was not properly initialised and disposed of.
> > 2. using libxl_retrieve_domain_configuration caused thread to
> >    deadlock itself.
> > 
> > Fix those issues by:
> > 
> > 1. properly initialise and dispose of d_config.
> > 2. switch to use libxl__get_domain_configuration.
> > 
> > Note that in theory we can refactor libxl_retrieve_domain_configuration
> > a bit to get a function without locking, but up until the calculation of
> > extra memory only relies on static configuration, hence we use the
> > stored configuration only.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> I think we should probably do the patch below, too.  I've checked that
> it doesn't break the build (after your patch) :-).
> 
> Thanks,
> Ian.
> 
> From abe530fb827edb22ed3db51d56f07b88cf8f0e17 Mon Sep 17 00:00:00 2001
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> Date: Tue, 4 Oct 2016 10:19:36 +0100
> Subject: [PATCH] libxl: Mark libxl_retrieve_domain_configuration as for
>  external callers only
> 
> This function takes the userdata lock.  Incautious use inside libxl
> can result in nested acquisition of that lock, and deadlock.
> 
> There is no good reason to use this function inside libxl, but it is a
> superficially attractive option.  Make future regressions easier to
> spot by marking the function for external use only.
> 
> Similar arguments apply for the application-facing userdata accessors,
> so do those too.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

I thought about doing the same thing yesterday.

I will commit both patches soon.

> ---
>  tools/libxl/libxl.h | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 969a089..acbf476 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -1352,7 +1352,8 @@ void libxl_domain_config_dispose(libxl_domain_config *d_config);
>   * works with DomU.
>   */
>  int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
> -                                        libxl_domain_config *d_config);
> +                                        libxl_domain_config *d_config)
> +                                        LIBXL_EXTERNAL_CALLERS_ONLY;
>  
>  int libxl_domain_suspend(libxl_ctx *ctx, uint32_t domid, int fd,
>                           int flags, /* LIBXL_SUSPEND_* */
> @@ -1945,12 +1946,14 @@ void libxl_cpuid_set(libxl_ctx *ctx, uint32_t domid,
>   */
>  int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
>                                const char *userdata_userid,
> -                              const uint8_t *data, int datalen);
> +                              const uint8_t *data, int datalen)
> +                              LIBXL_EXTERNAL_CALLERS_ONLY;
>    /* If datalen==0, data is not used and the user data for
>     * that domain and userdata_userid is deleted. */
>  int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid,
>                                   const char *userdata_userid,
> -                                 uint8_t **data_r, int *datalen_r);
> +                                 uint8_t **data_r, int *datalen_r)
> +                                 LIBXL_EXTERNAL_CALLERS_ONLY;
>    /* On successful return, *data_r is from malloc.
>     * If there is no data for that domain and userdata_userid,
>     * *data_r and *datalen_r will be set to 0.
> -- 
> 2.1.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel