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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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.