[PATCH 0/2] xl: event machinery fixes

[PATCH 0/2] xl: event machinery fixes

[Wei Liu]

Wei Liu (2):
  xl: free event in DOMAIN_RESTART_RENAME error path
  xl: disable events earlier for shutdown event

 tools/libxl/xl_cmdimpl.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

-- 
2.11.0


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

[PATCH 1/2] xl: free event in DOMAIN_RESTART_RENAME error path

[Wei Liu]

Otherwise it is leaked. Found by code inspection.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Fatih Acar <fatih.acar@gandi.net>

Backport candidate.
---
 tools/libxl/xl_cmdimpl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 7e8a8ae5c4..9bf10dfcb2 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3145,6 +3145,7 @@ start:
             case DOMAIN_RESTART_RENAME:
                 if (domid_soft_reset == INVALID_DOMID &&
                     !preserve_domain(&domid, event, &d_config)) {
+                    libxl_event_free(event);
                     /* If we fail then exit leaving the old domain in place. */
                     ret = -1;
                     goto out;
-- 
2.11.0


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

Re: [PATCH 1/2] xl: free event in DOMAIN_RESTART_RENAME error path

[Wei Liu]

On Thu, Feb 02, 2017 at 03:46:36PM +0000, Wei Liu wrote:
> Otherwise it is leaked. Found by code inspection.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Fatih Acar <fatih.acar@gandi.net>
> 
> Backport candidate.
> ---
>  tools/libxl/xl_cmdimpl.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 7e8a8ae5c4..9bf10dfcb2 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3145,6 +3145,7 @@ start:
>              case DOMAIN_RESTART_RENAME:
>                  if (domid_soft_reset == INVALID_DOMID &&
>                      !preserve_domain(&domid, event, &d_config)) {
> +                    libxl_event_free(event);

Stupid me.

This should have been

 libxl_event_free(ctx, event);

>                      /* If we fail then exit leaving the old domain in place. */
>                      ret = -1;
>                      goto out;
> -- 
> 2.11.0
> 

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

Re: [PATCH 1/2] xl: free event in DOMAIN_RESTART_RENAME error path

[Ian Jackson]

Wei Liu writes ("[PATCH 1/2] xl: free event in DOMAIN_RESTART_RENAME error path"):
> Otherwise it is leaked. Found by code inspection.

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

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

[PATCH 2/2] xl: disable events earlier for shutdown event

[Wei Liu]

We need to disable event machinery when the guest shuts down.  It
doesn't really matter where we disable it as long as it is within the
branch for shutdown event.

Move the code snippet before handle_domain_death, so that d_config is
not yet updated and we have the correct ->num_disks. And the free diskws
and set it to NULL, so that diskws gets automatically set up again when
xl goes over the domain creation code path.

Reported-by: Fatih Acar <fatih.acar@gandi.net>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Fatih Acar <fatih.acar@gandi.net>

Fatih, please test this series to see if it fixes your issue.

Ian, backport candidate?
---
 tools/libxl/xl_cmdimpl.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 9bf10dfcb2..7df6db1ec9 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3137,6 +3137,11 @@ start:
             LOG("Domain %u has shut down, reason code %d 0x%x", domid,
                 event->u.domain_shutdown.shutdown_reason,
                 event->u.domain_shutdown.shutdown_reason);
+            libxl_evdisable_domain_death(ctx, deathw);
+            deathw = NULL;
+            evdisable_disk_ejects(diskws, d_config.num_disks);
+            free(diskws);
+            diskws = NULL;
             switch (handle_domain_death(&domid, event, &d_config)) {
             case DOMAIN_RESTART_SOFT_RESET:
                 domid_soft_reset = domid;
@@ -3154,9 +3159,6 @@ start:
                 /* Otherwise fall through and restart. */
             case DOMAIN_RESTART_NORMAL:
                 libxl_event_free(ctx, event);
-                libxl_evdisable_domain_death(ctx, deathw);
-                deathw = NULL;
-                evdisable_disk_ejects(diskws, d_config.num_disks);
                 /* discard any other events which may have been generated */
                 while (!(ret = libxl_event_check(ctx, &event,
                                                  LIBXL_EVENTMASK_ALL, 0,0))) {
-- 
2.11.0


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

Re: [PATCH 2/2] xl: disable events earlier for shutdown event

[Ian Jackson]

Wei Liu writes ("[PATCH 2/2] xl: disable events earlier for shutdown event"):
> We need to disable event machinery when the guest shuts down.  It
> doesn't really matter where we disable it as long as it is within the
> branch for shutdown event.

I don't think this is necessary.  My intent was that libxl_ctx_free
would automatically disable all event generation, and that it is
therefore not necessary to call _disable.

This isn't documented particularly clearly, but libxl_event.h says:

  * An evgen is associated with the libxl_ctx used for its creation.
  * After libxl_ctx_free, all corresponding evgen handles become
  * invalid and must no longer be passed to evdisable.

This implies it's legal to call libxl_ctx_free with some events
enabled.

But I think I don't really understand what the original bug is.

Ian.

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

Re: [PATCH 2/2] xl: disable events earlier for shutdown event

[Wei Liu]

On Thu, Feb 02, 2017 at 03:52:41PM +0000, Ian Jackson wrote:
> Wei Liu writes ("[PATCH 2/2] xl: disable events earlier for shutdown event"):
> > We need to disable event machinery when the guest shuts down.  It
> > doesn't really matter where we disable it as long as it is within the
> > branch for shutdown event.
> 
> I don't think this is necessary.  My intent was that libxl_ctx_free
> would automatically disable all event generation, and that it is
> therefore not necessary to call _disable.
> 
> This isn't documented particularly clearly, but libxl_event.h says:
> 
>   * An evgen is associated with the libxl_ctx used for its creation.
>   * After libxl_ctx_free, all corresponding evgen handles become
>   * invalid and must no longer be passed to evdisable.
> 
> This implies it's legal to call libxl_ctx_free with some events
> enabled.
> 

That's right. But this is not saying there is bug in the event
machinery.

> But I think I don't really understand what the original bug is.
> 

The original bug is that:

1. boot a guest with no disks, so diskws is NULL, num_disks == 0
2. some removable disks are added, and domain config updated
3. guest reboots, xl gets shutdown event
4. handle_domain_death gets the latest d_config, num_disks != 0 now
5. try to disable disk eject events with evdisable_disk_ejects -> BOOM

So basically 5 needs to happen before 4. Moving that snippet seems to do
the trick, and then freeing diskws + setting it to NULL makes the
reboot path automatically re-register diskws.

Wei.

> Ian.

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

Re: [PATCH 2/2] xl: disable events earlier for shutdown event

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 2/2] xl: disable events earlier for shutdown event"):
> On Thu, Feb 02, 2017 at 03:52:41PM +0000, Ian Jackson wrote:
> > But I think I don't really understand what the original bug is.
> 
> The original bug is that:

Ah.

> 1. boot a guest with no disks, so diskws is NULL, num_disks == 0
> 2. some removable disks are added, and domain config updated
> 3. guest reboots, xl gets shutdown event
> 4. handle_domain_death gets the latest d_config, num_disks != 0 now
> 5. try to disable disk eject events with evdisable_disk_ejects -> BOOM
> 
> So basically 5 needs to happen before 4. Moving that snippet seems to do
> the trick, and then freeing diskws + setting it to NULL makes the
> reboot path automatically re-register diskws.

Surely the right answer is to make evdisable_disk_ejects tolerate
whatever it finds in diskws (including diskws==NULL).  It could be
idempotent too.

That way nothing depends on the exact ordering.

Ian.

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

Re: [PATCH 2/2] xl: disable events earlier for shutdown event

[Wei Liu]

On Thu, Feb 02, 2017 at 04:05:08PM +0000, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH 2/2] xl: disable events earlier for shutdown event"):
> > On Thu, Feb 02, 2017 at 03:52:41PM +0000, Ian Jackson wrote:
> > > But I think I don't really understand what the original bug is.
> > 
> > The original bug is that:
> 
> Ah.
> 
> > 1. boot a guest with no disks, so diskws is NULL, num_disks == 0
> > 2. some removable disks are added, and domain config updated
> > 3. guest reboots, xl gets shutdown event
> > 4. handle_domain_death gets the latest d_config, num_disks != 0 now
> > 5. try to disable disk eject events with evdisable_disk_ejects -> BOOM
> > 
> > So basically 5 needs to happen before 4. Moving that snippet seems to do
> > the trick, and then freeing diskws + setting it to NULL makes the
> > reboot path automatically re-register diskws.
> 
> Surely the right answer is to make evdisable_disk_ejects tolerate
> whatever it finds in diskws (including diskws==NULL).  It could be
> idempotent too.
> 

No, handling NULL is not enough. It could well be possible that diskws
is not NULL but then num_disks grows, thus making evdisable_disk_ejects
access out of bound.

The other solution is to have a dedicated counter for diskws instead of
relying on num_disks.

---8<---
From 43dc119327923ad6237e2d4d7bde5ec1147d9007 Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Thu, 2 Feb 2017 16:16:53 +0000
Subject: [PATCH] xl: track size of diskws with a dedicated counter

The num_disks field can change during guest lifetime. Don't use that as
the size of diskws, use a dedicated counter instead.

Also free diskws and reset diskws to NULL after disabling events so that
it will be automatically re-created when the guest reboots.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index b25ac6efa0..358757fd09 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2811,6 +2811,7 @@ static int create_domain(struct domain_create *dom_info)
     int ret, rc;
     libxl_evgen_domain_death *deathw = NULL;
     libxl_evgen_disk_eject **diskws = NULL; /* one per disk */
+    unsigned int num_diskws = 0;
     void *config_data = 0;
     int config_len = 0;
     int restore_fd = -1;
@@ -3119,8 +3120,9 @@ start:
         diskws = xmalloc(sizeof(*diskws) * d_config.num_disks);
         for (i = 0; i < d_config.num_disks; i++)
             diskws[i] = NULL;
+        num_diskws = d_config.num_disks;
     }
-    for (i = 0; i < d_config.num_disks; i++) {
+    for (i = 0; i < num_diskws; i++) {
         if (d_config.disks[i].removable) {
             ret = libxl_evenable_disk_eject(ctx, domid, d_config.disks[i].vdev,
                                             0, &diskws[i]);
@@ -3157,7 +3159,10 @@ start:
                 libxl_event_free(ctx, event);
                 libxl_evdisable_domain_death(ctx, deathw);
                 deathw = NULL;
-                evdisable_disk_ejects(diskws, d_config.num_disks);
+                evdisable_disk_ejects(diskws, num_diskws);
+                free(diskws);
+                diskws = NULL;
+                num_diskws = 0;
                 /* discard any other events which may have been generated */
                 while (!(ret = libxl_event_check(ctx, &event,
                                                  LIBXL_EVENTMASK_ALL, 0,0))) {
-- 
2.11.0


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

Re: [PATCH 2/2] xl: disable events earlier for shutdown event

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 2/2] xl: disable events earlier for shutdown event"):
> No, handling NULL is not enough. It could well be possible that diskws
> is not NULL but then num_disks grows, thus making evdisable_disk_ejects
> access out of bound.

The additional diskws entries could be zeroed then.  But, ...

> The other solution is to have a dedicated counter for diskws instead of
> relying on num_disks.

... this is a good approach too.

> Subject: [PATCH] xl: track size of diskws with a dedicated counter
> 
> The num_disks field can change during guest lifetime. Don't use that as
> the size of diskws, use a dedicated counter instead.
> 
> Also free diskws and reset diskws to NULL after disabling events so that
> it will be automatically re-created when the guest reboots.

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

Ian.

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

Re: [PATCH 2/2] xl: disable events earlier for shutdown event

[Fatih Acar]

On 02/02/2017 17:22, Wei Liu wrote:
> 
> The other solution is to have a dedicated counter for diskws instead of
> relying on num_disks.
> 

It's all good, both patches fix the issue, thank you.

-- 
Fatih ACAR
Gandi
fatih.acar@gandi.net

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

Re: [PATCH 2/2] xl: disable events earlier for shutdown event

[Wei Liu]

On Fri, Feb 03, 2017 at 10:46:14AM +0100, Fatih Acar wrote:
> On 02/02/2017 17:22, Wei Liu wrote:
> > 
> > The other solution is to have a dedicated counter for diskws instead of
> > relying on num_disks.
> > 
> 
> It's all good, both patches fix the issue, thank you.
> 

Cool, I will add a Tested-by tag.

> -- 
> Fatih ACAR
> Gandi
> fatih.acar@gandi.net

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

Re: [PATCH 0/2] xl: event machinery fixes

[Wei Liu]

On Thu, Feb 02, 2017 at 03:46:35PM +0000, Wei Liu wrote:
> Wei Liu (2):
>   xl: free event in DOMAIN_RESTART_RENAME error path
>   xl: disable events earlier for shutdown event
> 
>  tools/libxl/xl_cmdimpl.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Pushed the most up to date patches to staging.

> 
> -- 
> 2.11.0
> 

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