Re: [Xen-devel] [PATCH] libxl: fix issues in 38cd0664

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxx>

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

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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel