[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] xl: Fix segfault on domain reboot



On Mon, Jan 30, 2017 at 03:31:49PM +0100, Fatih Acar wrote:
> If we have no disk attached at startup, diskws is left unallocated
> but `d_config.num_disks` may change if we attach a disk later.
> When a domain is rebooted `evdisable_disk_ejects` is called
> this will later result in a segfault if num_disks has changed.
> 
> Expand diskws when num_disks increases.
> 
> Signed-off-by: Fatih Acar <fatih.acar@xxxxxxxxx>
> Signed-off-by: Nikita Kozlov <nikita.kozlov@xxxxxxxxx>
> Signed-off-by: Vincent Legout <vincent.legout@xxxxxxxxx>
> Signed-off-by: Baptiste Daroussin <baptiste.daroussin@xxxxxxxxx>


> ---
>  tools/libxl/xl_cmdimpl.c | 34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 7e8a8ae..f244e63 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -2517,13 +2517,25 @@ skip_usbdev:
>      xlu_cfg_destroy(config);
>  }
>  
> +static void realloc_diskws(libxl_evgen_disk_eject ***diskws, int old_count, 
> int new_count)
> +{
> +    libxl_evgen_disk_eject **diskws_new;
> +
> +    diskws_new = xcalloc(new_count, sizeof(*diskws_new));
> +    memcpy(diskws_new, *diskws, sizeof(**diskws) * old_count);
> +    free(*diskws);
> +    *diskws = diskws_new;

You didn't check if diskws_new is NULL.

Actually you can just use xrealloc here.

> +}
> +
>  static void reload_domain_config(uint32_t domid,
> -                                 libxl_domain_config *d_config)
> +                                 libxl_domain_config *d_config,
> +                                 libxl_evgen_disk_eject ***diskws)
>  {
>      int rc;
>      uint8_t *t_data;
>      int ret, t_len;
>      libxl_domain_config d_config_new;
> +    int disk_count;
>  
>      /* In case user has used "config-update" to store a new config
>       * file.
> @@ -2534,9 +2546,14 @@ static void reload_domain_config(uint32_t domid,
>      }
>      if (t_len > 0) {
>          LOG("\"xl\" configuration found, using it\n");
> +        disk_count = d_config->num_disks;
>          libxl_domain_config_dispose(d_config);
>          parse_config_data("<updated>", (const char *)t_data,
>                            t_len, d_config);
> +        if (d_config->num_disks > disk_count) {
> +            /* reallocate bigger diskws */
> +            realloc_diskws(diskws, disk_count, d_config->num_disks);
> +        }

It seems that you've used "xl config-update" to update the domain
configuration. Is this correct?

But actually we might want to fix the other code path as well.

Please give me some time to go over the code path to see if there is a
better approach than this patch. 

Wei.

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.