| Message ID | 20200612105519.18728-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-4.14] tools/libxc: Drop config_transformed parameter from xc_cpuid_set() | expand |
> -----Original Message----- > From: Andrew Cooper <andrew.cooper3@citrix.com> > Sent: 12 June 2020 11:55 > To: Xen-devel <xen-devel@lists.xenproject.org> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu > <wl@xen.org>; Paul Durrant <paul@xen.org> > Subject: [PATCH for-4.14] tools/libxc: Drop config_transformed parameter from xc_cpuid_set() > > libxl is now the sole caller of xc_cpuid_set(). The config_transformed output > is ignored, and this patch trivially highlights the resulting memory leak. > > "transformed" config is now properly forwarded on migrate as part of the > general VM state, so delete the transformation logic completely, rather than > trying to adjust just libxl to avoid leaking memory. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Paul Durrant <paul@xen.org> Release-acked-by: Paul Durrant <paul@xen.org> > --- > CC: Ian Jackson <Ian.Jackson@citrix.com> > CC: Wei Liu <wl@xen.org> > CC: Paul Durrant <paul@xen.org> > > For 4.14 for hopefully obvious reasons. > > Ian: for backport to 4.13 and earlier, there are a number of options. The > reasoning we used to delete the other callers of xc_cpuid_set() is still > valid, but probably not backport material. > > OTOH, moding libxl_cpuid_set() (as it was back then) to loop over cpuid_res[] > and free them all should work. > --- > tools/libxc/include/xenctrl.h | 3 +-- > tools/libxc/xc_cpuid_x86.c | 25 +------------------------ > tools/libxl/libxl_cpuid.c | 3 +-- > 3 files changed, 3 insertions(+), 28 deletions(-) > > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h > index f9e17ae424..113ddd935d 100644 > --- a/tools/libxc/include/xenctrl.h > +++ b/tools/libxc/include/xenctrl.h > @@ -1795,8 +1795,7 @@ int xc_domain_debug_control(xc_interface *xch, > int xc_cpuid_set(xc_interface *xch, > uint32_t domid, > const unsigned int *input, > - const char **config, > - char **config_transformed); > + const char **config); > > /* > * Make adjustments to the CPUID settings for a domain. > diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c > index 89d2ecdad2..b42edd6457 100644 > --- a/tools/libxc/xc_cpuid_x86.c > +++ b/tools/libxc/xc_cpuid_x86.c > @@ -279,7 +279,7 @@ int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid, > */ > int xc_cpuid_set( > xc_interface *xch, uint32_t domid, const unsigned int *input, > - const char **config, char **config_transformed) > + const char **config) > { > int rc; > unsigned int i, j, regs[4] = {}, polregs[4] = {}; > @@ -288,9 +288,6 @@ int xc_cpuid_set( > unsigned int nr_leaves, policy_leaves, nr_msrs; > uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1; > > - for ( i = 0; i < 4; ++i ) > - config_transformed[i] = NULL; > - > if ( xc_domain_getinfo(xch, domid, 1, &di) != 1 || > di.domid != domid ) > { > @@ -365,13 +362,6 @@ int xc_cpuid_set( > continue; > } > > - config_transformed[i] = calloc(33, 1); /* 32 bits, NUL terminator. */ > - if ( config_transformed[i] == NULL ) > - { > - rc = -ENOMEM; > - goto fail; > - } > - > /* > * Notes for following this algorithm: > * > @@ -399,11 +389,6 @@ int xc_cpuid_set( > set_feature(31 - j, regs[i]); > else > clear_feature(31 - j, regs[i]); > - > - config_transformed[i][j] = config[i][j]; > - /* All non 0/1 values get overwritten. */ > - if ( (config[i][j] & ~1) != '0' ) > - config_transformed[i][j] = '0' + val; > } > } > > @@ -421,16 +406,8 @@ int xc_cpuid_set( > } > > /* Success! */ > - goto out; > > fail: > - for ( i = 0; i < 4; i++ ) > - { > - free(config_transformed[i]); > - config_transformed[i] = NULL; > - } > - > - out: > free(leaves); > > return rc; > diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c > index 4e4852ddeb..796ec4f2d9 100644 > --- a/tools/libxl/libxl_cpuid.c > +++ b/tools/libxl/libxl_cpuid.c > @@ -421,7 +421,6 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, > { > libxl_cpuid_policy_list cpuid = info->cpuid; > int i; > - char *cpuid_res[4]; > bool pae = true; > > /* > @@ -444,7 +443,7 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, > > for (i = 0; cpuid[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) > xc_cpuid_set(ctx->xch, domid, cpuid[i].input, > - (const char**)(cpuid[i].policy), cpuid_res); > + (const char**)cpuid[i].policy); > } > > static const char *input_names[2] = { "leaf", "subleaf" }; > -- > 2.11.0
Paul Durrant writes ("RE: [PATCH for-4.14] tools/libxc: Drop config_transformed parameter from xc_cpuid_set()"):
> > -----Original Message-----
> > From: Andrew Cooper <andrew.cooper3@citrix.com>
> > Sent: 12 June 2020 11:55
> > To: Xen-devel <xen-devel@lists.xenproject.org>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu
> > <wl@xen.org>; Paul Durrant <paul@xen.org>
> > Subject: [PATCH for-4.14] tools/libxc: Drop config_transformed parameter from xc_cpuid_set()
> >
> > libxl is now the sole caller of xc_cpuid_set(). The config_transformed output
> > is ignored, and this patch trivially highlights the resulting memory leak.
> >
> > "transformed" config is now properly forwarded on migrate as part of the
> > general VM state, so delete the transformation logic completely, rather than
> > trying to adjust just libxl to avoid leaking memory.
> >
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Reviewed-by: Paul Durrant <paul@xen.org>
> Release-acked-by: Paul Durrant <paul@xen.org>
Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
I will commit this in a moment.
Ian.
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index f9e17ae424..113ddd935d 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -1795,8 +1795,7 @@ int xc_domain_debug_control(xc_interface *xch, int xc_cpuid_set(xc_interface *xch, uint32_t domid, const unsigned int *input, - const char **config, - char **config_transformed); + const char **config); /* * Make adjustments to the CPUID settings for a domain. diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c index 89d2ecdad2..b42edd6457 100644 --- a/tools/libxc/xc_cpuid_x86.c +++ b/tools/libxc/xc_cpuid_x86.c @@ -279,7 +279,7 @@ int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid, */ int xc_cpuid_set( xc_interface *xch, uint32_t domid, const unsigned int *input, - const char **config, char **config_transformed) + const char **config) { int rc; unsigned int i, j, regs[4] = {}, polregs[4] = {}; @@ -288,9 +288,6 @@ int xc_cpuid_set( unsigned int nr_leaves, policy_leaves, nr_msrs; uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1; - for ( i = 0; i < 4; ++i ) - config_transformed[i] = NULL; - if ( xc_domain_getinfo(xch, domid, 1, &di) != 1 || di.domid != domid ) { @@ -365,13 +362,6 @@ int xc_cpuid_set( continue; } - config_transformed[i] = calloc(33, 1); /* 32 bits, NUL terminator. */ - if ( config_transformed[i] == NULL ) - { - rc = -ENOMEM; - goto fail; - } - /* * Notes for following this algorithm: * @@ -399,11 +389,6 @@ int xc_cpuid_set( set_feature(31 - j, regs[i]); else clear_feature(31 - j, regs[i]); - - config_transformed[i][j] = config[i][j]; - /* All non 0/1 values get overwritten. */ - if ( (config[i][j] & ~1) != '0' ) - config_transformed[i][j] = '0' + val; } } @@ -421,16 +406,8 @@ int xc_cpuid_set( } /* Success! */ - goto out; fail: - for ( i = 0; i < 4; i++ ) - { - free(config_transformed[i]); - config_transformed[i] = NULL; - } - - out: free(leaves); return rc; diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c index 4e4852ddeb..796ec4f2d9 100644 --- a/tools/libxl/libxl_cpuid.c +++ b/tools/libxl/libxl_cpuid.c @@ -421,7 +421,6 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, { libxl_cpuid_policy_list cpuid = info->cpuid; int i; - char *cpuid_res[4]; bool pae = true; /* @@ -444,7 +443,7 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, for (i = 0; cpuid[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) xc_cpuid_set(ctx->xch, domid, cpuid[i].input, - (const char**)(cpuid[i].policy), cpuid_res); + (const char**)cpuid[i].policy); } static const char *input_names[2] = { "leaf", "subleaf" };
libxl is now the sole caller of xc_cpuid_set(). The config_transformed output is ignored, and this patch trivially highlights the resulting memory leak. "transformed" config is now properly forwarded on migrate as part of the general VM state, so delete the transformation logic completely, rather than trying to adjust just libxl to avoid leaking memory. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Ian Jackson <Ian.Jackson@citrix.com> CC: Wei Liu <wl@xen.org> CC: Paul Durrant <paul@xen.org> For 4.14 for hopefully obvious reasons. Ian: for backport to 4.13 and earlier, there are a number of options. The reasoning we used to delete the other callers of xc_cpuid_set() is still valid, but probably not backport material. OTOH, moding libxl_cpuid_set() (as it was back then) to loop over cpuid_res[] and free them all should work. --- tools/libxc/include/xenctrl.h | 3 +-- tools/libxc/xc_cpuid_x86.c | 25 +------------------------ tools/libxl/libxl_cpuid.c | 3 +-- 3 files changed, 3 insertions(+), 28 deletions(-)