[PATCH v1] libxl: prepare environment for domcreate_stream_done

[PATCH v1] libxl: prepare environment for domcreate_stream_done

[Olaf Hering]

The function domcreate_bootloader_done may branch early to
domcreate_stream_done, in case some error occoured. Here srs->dcs will be
NULL, which leads to a crash.

It is unclear what the purpose of that backpointer is. Perhaps it can be
removed, and domcreate_stream_done could use CONTAINER_OF.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 tools/libxl/libxl_create.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index a4e74a5cd2..989ce6d5bd 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1093,6 +1093,10 @@ static void domcreate_bootloader_done(libxl__egc *egc,
         return;
     }
 
+    /* Prepare environment for domcreate_stream_done */
+    if (!dcs->srs.dcs)
+        dcs->srs.dcs = dcs;
+
     /* Restore */
     callbacks->restore_results = libxl__srm_callout_callback_restore_results;
 
@@ -1116,7 +1120,6 @@ static void domcreate_bootloader_done(libxl__egc *egc,
         goto out;
 
     dcs->srs.ao = ao;
-    dcs->srs.dcs = dcs;
     dcs->srs.fd = restore_fd;
     dcs->srs.legacy = (dcs->restore_params.stream_version == 1);
     dcs->srs.back_channel = false;

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

Re: [PATCH v1] libxl: prepare environment for domcreate_stream_done

[Wei Liu]

Thanks for the patch!

On Thu, Mar 07, 2019 at 11:56:49AM +0100, Olaf Hering wrote:
> The function domcreate_bootloader_done may branch early to
> domcreate_stream_done, in case some error occoured. Here srs->dcs will be
> NULL, which leads to a crash.
> 
> It is unclear what the purpose of that backpointer is. Perhaps it can be
> removed, and domcreate_stream_done could use CONTAINER_OF.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>  tools/libxl/libxl_create.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index a4e74a5cd2..989ce6d5bd 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -1093,6 +1093,10 @@ static void domcreate_bootloader_done(libxl__egc *egc,
>          return;
>      }
>  
> +    /* Prepare environment for domcreate_stream_done */
> +    if (!dcs->srs.dcs)
> +        dcs->srs.dcs = dcs;
> +

Have you seen the field been set before entering this function? Or is
the if () just a precaution?

If it is the latter, can we have an ASSERT instead? Seeing the original
code already unconditionally assigns dcs, I don't think you can make it
any worse than before.

Wei.


>      /* Restore */
>      callbacks->restore_results = libxl__srm_callout_callback_restore_results;
>  
> @@ -1116,7 +1120,6 @@ static void domcreate_bootloader_done(libxl__egc *egc,
>          goto out;
>  
>      dcs->srs.ao = ao;
> -    dcs->srs.dcs = dcs;
>      dcs->srs.fd = restore_fd;
>      dcs->srs.legacy = (dcs->restore_params.stream_version == 1);
>      dcs->srs.back_channel = false;

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

Re: [PATCH v1] libxl: prepare environment for domcreate_stream_done

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 481 bytes --]

Am Thu, 7 Mar 2019 11:02:47 +0000
schrieb Wei Liu <wei.liu2@citrix.com>:

> Have you seen the field been set before entering this function? Or is
> the if () just a precaution?

Sorry, this is not the variant I intended to send.
The assignment was supposed to be unconditionally.

Today I browsed the code and it was not clear if that backpointer
is really required. Does anyone know if srs->dcs can ever be something
else than the dcs which contains that srs?

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH v1] libxl: prepare environment for domcreate_stream_done

[Wei Liu]

On Thu, Mar 07, 2019 at 12:53:38PM +0100, Olaf Hering wrote:
> Am Thu, 7 Mar 2019 11:02:47 +0000
> schrieb Wei Liu <wei.liu2@citrix.com>:
> 
> > Have you seen the field been set before entering this function? Or is
> > the if () just a precaution?
> 
> Sorry, this is not the variant I intended to send.
> The assignment was supposed to be unconditionally.
> 
> Today I browsed the code and it was not clear if that backpointer
> is really required. Does anyone know if srs->dcs can ever be something
> else than the dcs which contains that srs?

That code has been changed quite a bit with migration v2 and COLO. I
wouldn't be surprised if some code becomes stale.

Sadly I won't have time today to dig into the history.

Wei.

> 
> Olaf



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

Re: [PATCH v1] libxl: prepare environment for domcreate_stream_done

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 401 bytes --]

Am Thu, 7 Mar 2019 12:18:20 +0000
schrieb Wei Liu <wei.liu2@citrix.com>:

> That code has been changed quite a bit with migration v2 and COLO. I
> wouldn't be surprised if some code becomes stale.

Are you ok with just moving that assignment up to avoid the crash?
This should be backported to at least 4.10, older branches need that too.
A thoroughly review can still be done later.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH v1] libxl: prepare environment for domcreate_stream_done

[Wei Liu]

On Fri, Mar 08, 2019 at 10:22:18AM +0100, Olaf Hering wrote:
> Am Thu, 7 Mar 2019 12:18:20 +0000
> schrieb Wei Liu <wei.liu2@citrix.com>:
> 
> > That code has been changed quite a bit with migration v2 and COLO. I
> > wouldn't be surprised if some code becomes stale.
> 
> Are you ok with just moving that assignment up to avoid the crash?
> This should be backported to at least 4.10, older branches need that too.
> A thoroughly review can still be done later.
> 

Sure. That would be fine by me. Thanks for fixing this issue.

Wei.

> Olaf



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