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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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