[PATCH] xl/libxl: fix migrate/Remus regression (core dumped)

[PATCH] xl/libxl: fix migrate/Remus regression (core dumped)

[Yang Hongyang]

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.

Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_dom.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 74ea84b..8910b79 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1824,6 +1824,7 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
     port = xs_suspend_evtchn_port(dss->domid);
 
     if (port >= 0) {
+        libxl__ctx_evtchn_init(gc);
         dss->guest_evtchn.port =
             xc_suspend_evtchn_init_exclusive(CTX->xch, CTX->xce,
                                   dss->domid, port, &dss->guest_evtchn_lockfd);
-- 
1.7.1

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

[Ian Jackson]

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@eu.citrix.com>
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@cn.fujitsu.com>
    Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
    Cc: Ian Campbell <ian.campbell@citrix.com>
    Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
    Cc: Wei Liu <wei.liu2@citrix.com>

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.

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

[Ian Campbell]

On Mon, 2015-01-05 at 14:35 +0000, Ian Jackson wrote:
> 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@eu.citrix.com>
> 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@cn.fujitsu.com>
>     Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

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

[Konrad Rzeszutek Wilk]

On Mon, Jan 05, 2015 at 02:35:37PM +0000, Ian Jackson wrote:
> 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.

OK. Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

> 
> (I have also somewhat improved the English grammar in the commit
> message.)
> 
> Thanks,
> Ian.
> 
> commit 9d1cb27f5e961fd9db1c7d8381af18e33510f924
> Author: Ian Jackson <ian.jackson@eu.citrix.com>
> 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@cn.fujitsu.com>
>     Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
>     Cc: Ian Campbell <ian.campbell@citrix.com>
>     Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>     Cc: Wei Liu <wei.liu2@citrix.com>
> 
> 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.

Re: [PATCH for-4.5 v2] libxl: Initialise CTX->xce in domain suspend [and 2 more messages]

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH for-4.5 v2] libxl: Initialise CTX->xce in domain suspend"):
> Konrad: this should go in 4.5 because it is a bugfix without which
> libxl may dereference NULL.
...

Ian Campbell writes ("Re: [PATCH for-4.5 v2] libxl: Initialise CTX->xce in domain suspend"):
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Konrad Rzeszutek Wilk writes ("Re: [PATCH for-4.5 v2] libxl: Initialise CTX->xce in domain suspend"):
> OK. Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Applied, thanks.

Ian.