| Message ID | 22515.31497.619325.800023@mariner.uk.xensource.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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.
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(-)