Re: [Xen-devel] [PATCH for-4.5 v2] libxl: Initialise CTX->xce in domain suspend

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Yang Hongyang writes ("[PATCH] xl/libxl: fix migrate/Remus regression (core 
dumped)"):
> When excuting xl migrate/Remus, the following error occurd:
> [root@master xen]# xl migrate 5 slaver
> migration target: Ready to receive domain.
> Saving to migration stream new xl format (info 0x1/0x0/1225)
> Loading new save file <incoming migration stream> (new xl fmt info 
> 0x1/0x0/1225)
>  Savefile contains xl domain config in JSON format
> Parsing config from <saved>
> Segmentation fault (core dumped)
> 
> This is because CTX->xce is used without been initialized.
> The bug was introduced by commit 2ffeb5d7f5d8
>     libxl: events: Deregister evtchn fd when not needed
> which remove the initialization of xce from libxl__ctx_alloc.
> 
> This patch initialze the CTX->xce before use it.

Thanks.  This patch goes in the right direction, but isn't quite
correct because it doesn't check the return value from
libxl__ctx_evtchn_init.

Looking at this it is clear that following the on-demand
initialisation of CTX->xce, it is normally necessary for any evtchn
user in libxl to call libxl__ctx_evtchn_init, since they will need the
xce for finding the right port number to pass to
libxl__ev_evtchn_wait.

Sorry for not noticing this when I made my earlier change.

I have therefore:
 * In the patch below, added changes to the comments to document this.
 * Done git grep '\bxce\b' tools/libxl  and checked the other uses.
 * Consequently, verified that the rest of the code in libxl_dom.c
   avoids using xce unless guest_evtchn.port>=0, and properly
   initialises .port to -1, so that there is no need for further calls
   to libxl__ctx_evtchn_init.

I have compiled but not executed this patch.  Yang Hongyang: can you
please test that it fixes the bug for you ?

Konrad: this should go in 4.5 because it is a bugfix without which
libxl may dereference NULL.

(I have also somewhat improved the English grammar in the commit
message.)

Thanks,
Ian.

commit 9d1cb27f5e961fd9db1c7d8381af18e33510f924
Author: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Date:   Mon Jan 5 14:31:00 2015 +0000

    libxl: Initialise CTX->xce in domain suspend, as needed
    
    When excuting xl migrate/Remus, the following error can occur:
      [root@master xen]# xl migrate 5 slaver
      migration target: Ready to receive domain.
      Saving to migration stream new xl format (info 0x1/0x0/1225)
      Loading new save file <incoming migration stream> (new xl fmt info 
0x1/0x0/12\
    )
       Savefile contains xl domain config in JSON format
      Parsing config from <saved>
      Segmentation fault (core dumped)
    
    This is because CTX->xce is used without been initialized.
    The bug was introduced by commit 2ffeb5d7f5d8
        libxl: events: Deregister evtchn fd when not needed
    which removed the initialization of xce from libxl__ctx_alloc.
    
    In this patch we initialise the CTX->xce before using it.  Also, we
    adjust the doc comment for libxl__ev_evtchn_* to mention the need to
    do so.
    
    Signed-off-by: Yang Hongyang <yanghy@xxxxxxxxxxxxxx>
    Signed-off-by: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
    Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
    Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
    Cc: Wei Liu <wei.liu2@xxxxxxxxxx>

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 74ea84b..94ae818 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1824,6 +1824,9 @@ void libxl__domain_suspend(libxl__egc *egc, 
libxl__domain_suspend_state *dss)
     port = xs_suspend_evtchn_port(dss->domid);
 
     if (port >= 0) {
+        rc = libxl__ctx_evtchn_init(gc);
+        if (rc) goto out;
+
         dss->guest_evtchn.port =
             xc_suspend_evtchn_init_exclusive(CTX->xch, CTX->xce,
                                   dss->domid, port, &dss->guest_evtchn_lockfd);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9695f18..6dac0f8 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -800,8 +800,10 @@ static inline int libxl__ev_xswatch_isregistered(const 
libxl__ev_xswatch *xw)
 
 /*
  * The evtchn facility is one-shot per call to libxl__ev_evtchn_wait.
- * You should call some suitable xc bind function on (or to obtain)
- * the port, then libxl__ev_evtchn_wait.
+ * You should:
+ *   Use libxl__ctx_evtchn_init to make sure CTX->xce is valid;
+ *   Call some suitable xc bind function on (or to obtain) the port;
+ *   Then call libxl__ev_evtchn_wait.
  *
  * When the event is signaled then the callback will be made, once.
  * Then you must call libxl__ev_evtchn_wait again, if desired.

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