[PATCH 0/5] migration/remus: bug fix and cleanup

[PATCH 0/5] migration/remus: bug fix and cleanup

[Wen Congyang]

Wen Congyang (5):
  remus: don't call stream_continue() when doing failover
  remus: don't write xenstore data if it fails
  tools/libxc: don't send end record if remus fails
  tools/libxl: remove unused function libxl__domain_save_device_model()
  Allow all user to create a file under the directory /var/lib/xen

 tools/Makefile                   |  2 +-
 tools/libxc/xc_sr_save.c         |  2 +-
 tools/libxl/libxl_dom.c          | 91 ----------------------------------------
 tools/libxl/libxl_internal.h     |  3 --
 tools/libxl/libxl_stream_read.c  | 18 +++++---
 tools/libxl/libxl_stream_write.c |  8 +++-
 6 files changed, 21 insertions(+), 103 deletions(-)

-- 
2.5.0

[PATCH 1/5] remus: don't call stream_continue() when doing failover

[Wen Congyang]

stream_continue() is used for migration to read emulator
xenstore data and emulator context. For remus, if we do
failover, we have read it in the checkpoint cycle, and
we only need to complete the stream.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 tools/libxl/libxl_stream_read.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_stream_read.c b/tools/libxl/libxl_stream_read.c
index 258dec4..42c087f 100644
--- a/tools/libxl/libxl_stream_read.c
+++ b/tools/libxl/libxl_stream_read.c
@@ -758,6 +758,9 @@ void libxl__xc_domain_restore_done(libxl__egc *egc, void *dcs_void,
     libxl__stream_read_state *stream = &dcs->srs;
     STATE_AO_GC(dcs->ao);
 
+    /* convenience aliases */
+    const int checkpointed_stream = dcs->restore_params.checkpointed_stream;
+
     if (rc)
         goto err;
 
@@ -777,11 +780,16 @@ void libxl__xc_domain_restore_done(libxl__egc *egc, void *dcs_void,
      * If the stream is not still alive, we must not continue any work.
      */
     if (libxl__stream_read_inuse(stream)) {
-        /*
-         * Libxc has indicated that it is done with the stream.  Resume reading
-         * libxl records from it.
-         */
-        stream_continue(egc, stream);
+        if (checkpointed_stream) {
+            /* failover */
+            stream_complete(egc, stream, 0);
+        } else {
+            /*
+             * Libxc has indicated that it is done with the stream.
+             * Resume reading libxl records from it.
+             */
+            stream_continue(egc, stream);
+        }
     }
 }
 
-- 
2.5.0

Re: [PATCH 1/5] remus: don't call stream_continue() when doing failover

[Andrew Cooper]

On 30/12/2015 01:39, Wen Congyang wrote:
> stream_continue() is used for migration to read emulator
> xenstore data and emulator context. For remus, if we do
> failover, we have read it in the checkpoint cycle, and
> we only need to complete the stream.
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with one suggestion

> ---
>   tools/libxl/libxl_stream_read.c | 18 +++++++++++++-----
>   1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/tools/libxl/libxl_stream_read.c b/tools/libxl/libxl_stream_read.c
> index 258dec4..42c087f 100644
> --- a/tools/libxl/libxl_stream_read.c
> +++ b/tools/libxl/libxl_stream_read.c
> @@ -758,6 +758,9 @@ void libxl__xc_domain_restore_done(libxl__egc *egc, void *dcs_void,
>       libxl__stream_read_state *stream = &dcs->srs;
>       STATE_AO_GC(dcs->ao);
>   
> +    /* convenience aliases */
> +    const int checkpointed_stream = dcs->restore_params.checkpointed_stream;
> +
>       if (rc)
>           goto err;
>   
> @@ -777,11 +780,16 @@ void libxl__xc_domain_restore_done(libxl__egc *egc, void *dcs_void,
>        * If the stream is not still alive, we must not continue any work.
>        */
>       if (libxl__stream_read_inuse(stream)) {
> -        /*
> -         * Libxc has indicated that it is done with the stream.  Resume reading
> -         * libxl records from it.
> -         */
> -        stream_continue(egc, stream);
> +        if (checkpointed_stream) {
> +            /* failover */

I would recommend extending this comment to include the information in 
the commit message.  Something like:

/* Failover from primary.  Domain state is currently at a consistent 
checkpoint, ready to go. */

> +            stream_complete(egc, stream, 0);
> +        } else {
> +            /*
> +             * Libxc has indicated that it is done with the stream.
> +             * Resume reading libxl records from it.
> +             */
> +            stream_continue(egc, stream);
> +        }
>       }
>   }
>   

[PATCH 2/5] remus: don't write xenstore data if it fails

[Wen Congyang]

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 tools/libxl/libxl_stream_write.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_stream_write.c b/tools/libxl/libxl_stream_write.c
index 80d9208..ee9c53a 100644
--- a/tools/libxl/libxl_stream_write.c
+++ b/tools/libxl/libxl_stream_write.c
@@ -354,8 +354,12 @@ void libxl__xc_domain_save_done(libxl__egc *egc, void *dss_void,
      * alive, and check_all_finished() may have torn it down around us.
      * If the stream is not still alive, we must not continue any work.
      */
-    if (libxl__stream_write_inuse(stream))
-        write_emulator_xenstore_record(egc, stream);
+    if (libxl__stream_write_inuse(stream)) {
+        if (dss->remus)
+            stream_complete(egc, stream, 0);
+        else
+            write_emulator_xenstore_record(egc, stream);
+    }
 }
 
 static void write_emulator_xenstore_record(libxl__egc *egc,
-- 
2.5.0

Re: [PATCH 2/5] remus: don't write xenstore data if it fails

[Andrew Cooper]

On 30/12/2015 01:39, Wen Congyang wrote:

If what fails?

Given the content of this patch in the context of the previous one, I 
presume you are wishing to avoid re-sending the emulator state after 
libxl__xc_domain_save_done() completes, but under what circumstances 
will this occur?

> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>   tools/libxl/libxl_stream_write.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl_stream_write.c b/tools/libxl/libxl_stream_write.c
> index 80d9208..ee9c53a 100644
> --- a/tools/libxl/libxl_stream_write.c
> +++ b/tools/libxl/libxl_stream_write.c
> @@ -354,8 +354,12 @@ void libxl__xc_domain_save_done(libxl__egc *egc, void *dss_void,
>        * alive, and check_all_finished() may have torn it down around us.
>        * If the stream is not still alive, we must not continue any work.
>        */
> -    if (libxl__stream_write_inuse(stream))
> -        write_emulator_xenstore_record(egc, stream);
> +    if (libxl__stream_write_inuse(stream)) {
> +        if (dss->remus)
> +            stream_complete(egc, stream, 0);
> +        else
> +            write_emulator_xenstore_record(egc, stream);
> +    }
>   }
>   
>   static void write_emulator_xenstore_record(libxl__egc *egc,

Re: [PATCH 2/5] remus: don't write xenstore data if it fails

[Wen Congyang]

On 12/30/2015 06:47 PM, Andrew Cooper wrote:
> On 30/12/2015 01:39, Wen Congyang wrote:
> 
> If what fails?
> 
> Given the content of this patch in the context of the previous one, I presume you are wishing to avoid re-sending the emulator state after libxl__xc_domain_save_done() completes, but under what circumstances will this occur?

For example: if the secondary host is down, and we fail to send the data to
the secondary host. xc_domain_save() returns 0. So in the function
libxl__xc_domain_save_done(), rc is 0(the helper program exits normally),
and retval is 0(it is xc_domain_save()'s return value).

Thanks
Wen Congyang

> 
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> ---
>>   tools/libxl/libxl_stream_write.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_stream_write.c b/tools/libxl/libxl_stream_write.c
>> index 80d9208..ee9c53a 100644
>> --- a/tools/libxl/libxl_stream_write.c
>> +++ b/tools/libxl/libxl_stream_write.c
>> @@ -354,8 +354,12 @@ void libxl__xc_domain_save_done(libxl__egc *egc, void *dss_void,
>>        * alive, and check_all_finished() may have torn it down around us.
>>        * If the stream is not still alive, we must not continue any work.
>>        */
>> -    if (libxl__stream_write_inuse(stream))
>> -        write_emulator_xenstore_record(egc, stream);
>> +    if (libxl__stream_write_inuse(stream)) {
>> +        if (dss->remus)
>> +            stream_complete(egc, stream, 0);
>> +        else
>> +            write_emulator_xenstore_record(egc, stream);
>> +    }
>>   }
>>     static void write_emulator_xenstore_record(libxl__egc *egc,
> 
> 
> 
> .
> 

[PATCH 3/5] tools/libxc: don't send end record if remus fails

[Wen Congyang]

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 tools/libxc/xc_sr_save.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
index cefcef5..76ebb34 100644
--- a/tools/libxc/xc_sr_save.c
+++ b/tools/libxc/xc_sr_save.c
@@ -790,7 +790,7 @@ static int save(struct xc_sr_context *ctx, uint16_t guest_type)
 
             rc = ctx->save.callbacks->checkpoint(ctx->save.callbacks->data);
             if ( rc <= 0 )
-                ctx->save.checkpointed = false;
+                goto err;
         }
     } while ( ctx->save.checkpointed );
 
-- 
2.5.0

Re: [PATCH 3/5] tools/libxc: don't send end record if remus fails

[Andrew Cooper]

On 30/12/2015 01:39, Wen Congyang wrote:
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

While looking at this code, what about error handling for the postcopy() 
callback just out of context?

> ---
>   tools/libxc/xc_sr_save.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
> index cefcef5..76ebb34 100644
> --- a/tools/libxc/xc_sr_save.c
> +++ b/tools/libxc/xc_sr_save.c
> @@ -790,7 +790,7 @@ static int save(struct xc_sr_context *ctx, uint16_t guest_type)
>   
>               rc = ctx->save.callbacks->checkpoint(ctx->save.callbacks->data);
>               if ( rc <= 0 )
> -                ctx->save.checkpointed = false;
> +                goto err;
>           }
>       } while ( ctx->save.checkpointed );
>   

Re: [PATCH 3/5] tools/libxc: don't send end record if remus fails

[Wen Congyang]

On 12/30/2015 07:11 PM, Andrew Cooper wrote:
> On 30/12/2015 01:39, Wen Congyang wrote:
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> While looking at this code, what about error handling for the postcopy() callback just out of context?

We don't handle it now. IIRC, migration v1 doesn't handle it to. I think we should
handle it.

Thanks
Wen Congyang

> 
>> ---
>>   tools/libxc/xc_sr_save.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
>> index cefcef5..76ebb34 100644
>> --- a/tools/libxc/xc_sr_save.c
>> +++ b/tools/libxc/xc_sr_save.c
>> @@ -790,7 +790,7 @@ static int save(struct xc_sr_context *ctx, uint16_t guest_type)
>>                 rc = ctx->save.callbacks->checkpoint(ctx->save.callbacks->data);
>>               if ( rc <= 0 )
>> -                ctx->save.checkpointed = false;
>> +                goto err;
>>           }
>>       } while ( ctx->save.checkpointed );
>>   
> 
> 
> 
> .
> 

[PATCH 4/5] tools/libxl: remove unused function libxl__domain_save_device_model()

[Wen Congyang]

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 tools/libxl/libxl_dom.c      | 91 --------------------------------------------
 tools/libxl/libxl_internal.h |  3 --
 2 files changed, 94 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 47971a9..2269998 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1785,97 +1785,6 @@ static void stream_done(libxl__egc *egc,
     domain_save_done(egc, sws->dss, rc);
 }
 
-static void save_device_model_datacopier_done(libxl__egc *egc,
-     libxl__datacopier_state *dc, int rc, int onwrite, int errnoval);
-
-void libxl__domain_save_device_model(libxl__egc *egc,
-                                     libxl__domain_suspend_state *dss,
-                                     libxl__save_device_model_cb *callback)
-{
-    STATE_AO_GC(dss->ao);
-    struct stat st;
-    uint32_t qemu_state_len;
-    int rc;
-
-    dss->save_dm_callback = callback;
-
-    /* Convenience aliases */
-    const char *const filename = dss->dm_savefile;
-    const int fd = dss->fd;
-
-    libxl__datacopier_state *dc = &dss->save_dm_datacopier;
-    memset(dc, 0, sizeof(*dc));
-    dc->readwhat = GCSPRINTF("qemu save file %s", filename);
-    dc->ao = ao;
-    dc->readfd = -1;
-    dc->writefd = fd;
-    dc->maxsz = INT_MAX;
-    dc->bytes_to_read = -1;
-    dc->copywhat = GCSPRINTF("qemu save file for domain %"PRIu32, dss->domid);
-    dc->writewhat = "save/migration stream";
-    dc->callback = save_device_model_datacopier_done;
-
-    dc->readfd = open(filename, O_RDONLY);
-    if (dc->readfd < 0) {
-        LOGE(ERROR, "unable to open %s", dc->readwhat);
-        rc = ERROR_FAIL;
-        goto out;
-    }
-
-    if (fstat(dc->readfd, &st))
-    {
-        LOGE(ERROR, "unable to fstat %s", dc->readwhat);
-        rc = ERROR_FAIL;
-        goto out;
-    }
-
-    if (!S_ISREG(st.st_mode)) {
-        LOG(ERROR, "%s is not a plain file!", dc->readwhat);
-        rc = ERROR_FAIL;
-        goto out;
-    }
-
-    qemu_state_len = st.st_size;
-    LOG(DEBUG, "%s is %d bytes", dc->readwhat, qemu_state_len);
-
-    rc = libxl__datacopier_start(dc);
-    if (rc) goto out;
-
-    libxl__datacopier_prefixdata(egc, dc,
-                                 QEMU_SIGNATURE, strlen(QEMU_SIGNATURE));
-
-    libxl__datacopier_prefixdata(egc, dc,
-                                 &qemu_state_len, sizeof(qemu_state_len));
-    return;
-
- out:
-    save_device_model_datacopier_done(egc, dc, rc, -1, EIO);
-}
-
-static void save_device_model_datacopier_done(libxl__egc *egc,
-     libxl__datacopier_state *dc, int our_rc, int onwrite, int errnoval)
-{
-    libxl__domain_suspend_state *dss =
-        CONTAINER_OF(dc, *dss, save_dm_datacopier);
-    STATE_AO_GC(dss->ao);
-
-    /* Convenience aliases */
-    const char *const filename = dss->dm_savefile;
-    int rc;
-
-    libxl__datacopier_kill(dc);
-
-    if (dc->readfd >= 0) {
-        close(dc->readfd);
-        dc->readfd = -1;
-    }
-
-    rc = libxl__remove_file(gc, filename);
-    if (!our_rc) our_rc = rc;
-
-    dss->save_dm_callback(egc, dss, our_rc);
-}
-
 static void libxl__remus_teardown(libxl__egc *egc,
                                   libxl__domain_suspend_state *dss,
                                   int rc);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index a556a38..630172b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3498,9 +3498,6 @@ static inline bool libxl__save_helper_inuse(const libxl__save_helper_state *shs)
 /* Each time the dm needs to be saved, we must call suspend and then save */
 _hidden int libxl__domain_suspend_device_model(libxl__gc *gc,
                                            libxl__domain_suspend_state *dss);
-_hidden void libxl__domain_save_device_model(libxl__egc *egc,
-                                     libxl__domain_suspend_state *dss,
-                                     libxl__save_device_model_cb *callback);
 
 _hidden const char *libxl__device_model_savefile(libxl__gc *gc, uint32_t domid);
 
-- 
2.5.0

[PATCH 5/5] Allow all user to create a file under the directory /var/lib/xen

[Wen Congyang]

We may use non-root user to run qemu, and the qemu needs to write
save file to /var/lib/xen. So we should allow all user to create
a file under the directory /var/lib/xen

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 tools/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/Makefile b/tools/Makefile
index 820ca40..402b417 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -60,7 +60,7 @@ build all: subdirs-all
 install: subdirs-install
 	$(INSTALL_DIR) -m 700 $(DESTDIR)$(XEN_DUMP_DIR)
 	$(INSTALL_DIR) $(DESTDIR)/var/log/xen
-	$(INSTALL_DIR) $(DESTDIR)/var/lib/xen
+	$(INSTALL_DIR) -m 777 $(DESTDIR)/var/lib/xen
 
 .PHONY: uninstall
 uninstall: D=$(DESTDIR)
-- 
2.5.0

Re: [PATCH 5/5] Allow all user to create a file under the directory /var/lib/xen

[Doug Goldstein]

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

On 12/29/15 8:39 PM, Wen Congyang wrote:
> We may use non-root user to run qemu, and the qemu needs to write
> save file to /var/lib/xen. So we should allow all user to create
> a file under the directory /var/lib/xen
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>  tools/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/Makefile b/tools/Makefile
> index 820ca40..402b417 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -60,7 +60,7 @@ build all: subdirs-all
>  install: subdirs-install
>  	$(INSTALL_DIR) -m 700 $(DESTDIR)$(XEN_DUMP_DIR)
>  	$(INSTALL_DIR) $(DESTDIR)/var/log/xen
> -	$(INSTALL_DIR) $(DESTDIR)/var/lib/xen
> +	$(INSTALL_DIR) -m 777 $(DESTDIR)/var/lib/xen
>  
>  .PHONY: uninstall
>  uninstall: D=$(DESTDIR)
> 

I could be wrong but this doesn't seem like something that you'd want to
do given what's stored in there. Could you do something with permissions
on sub-directories to achieve what you need?

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

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

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

Re: [PATCH 5/5] Allow all user to create a file under the directory /var/lib/xen

[Wen Congyang]

On 12/30/2015 12:11 PM, Doug Goldstein wrote:
> On 12/29/15 8:39 PM, Wen Congyang wrote:
>> We may use non-root user to run qemu, and the qemu needs to write
>> save file to /var/lib/xen. So we should allow all user to create
>> a file under the directory /var/lib/xen
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> ---
>>  tools/Makefile | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/Makefile b/tools/Makefile
>> index 820ca40..402b417 100644
>> --- a/tools/Makefile
>> +++ b/tools/Makefile
>> @@ -60,7 +60,7 @@ build all: subdirs-all
>>  install: subdirs-install
>>  	$(INSTALL_DIR) -m 700 $(DESTDIR)$(XEN_DUMP_DIR)
>>  	$(INSTALL_DIR) $(DESTDIR)/var/log/xen
>> -	$(INSTALL_DIR) $(DESTDIR)/var/lib/xen
>> +	$(INSTALL_DIR) -m 777 $(DESTDIR)/var/lib/xen
>>  
>>  .PHONY: uninstall
>>  uninstall: D=$(DESTDIR)
>>
> 
> I could be wrong but this doesn't seem like something that you'd want to
> do given what's stored in there. Could you do something with permissions
> on sub-directories to achieve what you need?
> 

The save file's path is:
#define LIBXL_DEVICE_MODEL_SAVE_FILE "/var/lib/xen/qemu-save" /* .$domid */

So all user must have write permission on the directory /var/lib/xen/, otherwise,
the migration will fail.

Thanks
Wen Congyang

Re: [PATCH 5/5] Allow all user to create a file under the directory /var/lib/xen

[Andrew Cooper]

On 30/12/2015 05:25, Wen Congyang wrote:
> On 12/30/2015 12:11 PM, Doug Goldstein wrote:
>> On 12/29/15 8:39 PM, Wen Congyang wrote:
>>> We may use non-root user to run qemu, and the qemu needs to write
>>> save file to /var/lib/xen. So we should allow all user to create
>>> a file under the directory /var/lib/xen
>>>
>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>> ---
>>>   tools/Makefile | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tools/Makefile b/tools/Makefile
>>> index 820ca40..402b417 100644
>>> --- a/tools/Makefile
>>> +++ b/tools/Makefile
>>> @@ -60,7 +60,7 @@ build all: subdirs-all
>>>   install: subdirs-install
>>>   	$(INSTALL_DIR) -m 700 $(DESTDIR)$(XEN_DUMP_DIR)
>>>   	$(INSTALL_DIR) $(DESTDIR)/var/log/xen
>>> -	$(INSTALL_DIR) $(DESTDIR)/var/lib/xen
>>> +	$(INSTALL_DIR) -m 777 $(DESTDIR)/var/lib/xen
>>>   
>>>   .PHONY: uninstall
>>>   uninstall: D=$(DESTDIR)
>>>
>> I could be wrong but this doesn't seem like something that you'd want to
>> do given what's stored in there. Could you do something with permissions
>> on sub-directories to achieve what you need?
>>
> The save file's path is:
> #define LIBXL_DEVICE_MODEL_SAVE_FILE "/var/lib/xen/qemu-save" /* .$domid */
>
> So all user must have write permission on the directory /var/lib/xen/, otherwise,
> the migration will fail.

For now, I would avoid running qemu as a non-root user.  It doesn't gain 
you any meaninful security at present (at the expense of a warning which 
can't be turned off).

As to this bug, marking the directory 0777 is not an option, as save 
records necessarily contain sensitive data.

Longterm, (and already identified in one of the threads in the past), 
the best course of action is to switch away from having files, and 
passing file descriptors instead.  This is more flexible (currently 
libxl can't function on a read-only root filesystem), and would allow a 
privileged entity to open the file descriptor and pass it to a 
non-privileged entity to use.  This allows the non-privileged entity to 
function, and maintains security.

~Andrew

Re: [PATCH 5/5] Allow all user to create a file under the directory /var/lib/xen

[Konrad Rzeszutek Wilk]

On Wed, Dec 30, 2015 at 11:00:52AM +0000, Andrew Cooper wrote:
> On 30/12/2015 05:25, Wen Congyang wrote:
> >On 12/30/2015 12:11 PM, Doug Goldstein wrote:
> >>On 12/29/15 8:39 PM, Wen Congyang wrote:
> >>>We may use non-root user to run qemu, and the qemu needs to write
> >>>save file to /var/lib/xen. So we should allow all user to create
> >>>a file under the directory /var/lib/xen
> >>>
> >>>Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> >>>---
> >>>  tools/Makefile | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>>diff --git a/tools/Makefile b/tools/Makefile
> >>>index 820ca40..402b417 100644
> >>>--- a/tools/Makefile
> >>>+++ b/tools/Makefile
> >>>@@ -60,7 +60,7 @@ build all: subdirs-all
> >>>  install: subdirs-install
> >>>  	$(INSTALL_DIR) -m 700 $(DESTDIR)$(XEN_DUMP_DIR)
> >>>  	$(INSTALL_DIR) $(DESTDIR)/var/log/xen
> >>>-	$(INSTALL_DIR) $(DESTDIR)/var/lib/xen
> >>>+	$(INSTALL_DIR) -m 777 $(DESTDIR)/var/lib/xen
> >>>  .PHONY: uninstall
> >>>  uninstall: D=$(DESTDIR)
> >>>
> >>I could be wrong but this doesn't seem like something that you'd want to
> >>do given what's stored in there. Could you do something with permissions
> >>on sub-directories to achieve what you need?
> >>
> >The save file's path is:
> >#define LIBXL_DEVICE_MODEL_SAVE_FILE "/var/lib/xen/qemu-save" /* .$domid */
> >
> >So all user must have write permission on the directory /var/lib/xen/, otherwise,
> >the migration will fail.
> 
> For now, I would avoid running qemu as a non-root user.  It doesn't gain you
> any meaninful security at present (at the expense of a warning which can't
> be turned off).
> 
> As to this bug, marking the directory 0777 is not an option, as save records
> necessarily contain sensitive data.
> 
> Longterm, (and already identified in one of the threads in the past), the
> best course of action is to switch away from having files, and passing file
> descriptors instead.  This is more flexible (currently libxl can't function
> on a read-only root filesystem), and would allow a privileged entity to open
> the file descriptor and pass it to a non-privileged entity to use.  This
> allows the non-privileged entity to function, and maintains security.

Wen,

Could you mention the use case for wanting to write files there? Looking
at the patches you had sent for COLO and Remus they use an file descriptor - so
what is the use-case here?

Thanks!
> 
> ~Andrew
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 5/5] Allow all user to create a file under the directory /var/lib/xen

[Andrew Cooper]

On 25/01/2016 20:36, Konrad Rzeszutek Wilk wrote:
> On Wed, Dec 30, 2015 at 11:00:52AM +0000, Andrew Cooper wrote:
>> On 30/12/2015 05:25, Wen Congyang wrote:
>>> On 12/30/2015 12:11 PM, Doug Goldstein wrote:
>>>> On 12/29/15 8:39 PM, Wen Congyang wrote:
>>>>> We may use non-root user to run qemu, and the qemu needs to write
>>>>> save file to /var/lib/xen. So we should allow all user to create
>>>>> a file under the directory /var/lib/xen
>>>>>
>>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>>> ---
>>>>>  tools/Makefile | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/tools/Makefile b/tools/Makefile
>>>>> index 820ca40..402b417 100644
>>>>> --- a/tools/Makefile
>>>>> +++ b/tools/Makefile
>>>>> @@ -60,7 +60,7 @@ build all: subdirs-all
>>>>>  install: subdirs-install
>>>>>  	$(INSTALL_DIR) -m 700 $(DESTDIR)$(XEN_DUMP_DIR)
>>>>>  	$(INSTALL_DIR) $(DESTDIR)/var/log/xen
>>>>> -	$(INSTALL_DIR) $(DESTDIR)/var/lib/xen
>>>>> +	$(INSTALL_DIR) -m 777 $(DESTDIR)/var/lib/xen
>>>>>  .PHONY: uninstall
>>>>>  uninstall: D=$(DESTDIR)
>>>>>
>>>> I could be wrong but this doesn't seem like something that you'd want to
>>>> do given what's stored in there. Could you do something with permissions
>>>> on sub-directories to achieve what you need?
>>>>
>>> The save file's path is:
>>> #define LIBXL_DEVICE_MODEL_SAVE_FILE "/var/lib/xen/qemu-save" /* .$domid */
>>>
>>> So all user must have write permission on the directory /var/lib/xen/, otherwise,
>>> the migration will fail.
>> For now, I would avoid running qemu as a non-root user.  It doesn't gain you
>> any meaninful security at present (at the expense of a warning which can't
>> be turned off).
>>
>> As to this bug, marking the directory 0777 is not an option, as save records
>> necessarily contain sensitive data.
>>
>> Longterm, (and already identified in one of the threads in the past), the
>> best course of action is to switch away from having files, and passing file
>> descriptors instead.  This is more flexible (currently libxl can't function
>> on a read-only root filesystem), and would allow a privileged entity to open
>> the file descriptor and pass it to a non-privileged entity to use.  This
>> allows the non-privileged entity to function, and maintains security.
> Wen,
>
> Could you mention the use case for wanting to write files there? Looking
> at the patches you had sent for COLO and Remus they use an file descriptor - so
> what is the use-case here?

This is a bug in existing code.  It is not a COLO specific issue.

The current protocol for live migration requires Qemu to write its save
file here.

Until this issue is resolved, live migration is inoperable with Qemu
running as a non-root user.

~Andrew

Re: [PATCH 5/5] Allow all user to create a file under the directory /var/lib/xen

[Ian Campbell]

On Tue, 2016-01-26 at 00:00 +0000, Andrew Cooper wrote:
> On 25/01/2016 20:36, Konrad Rzeszutek Wilk wrote:
> > On Wed, Dec 30, 2015 at 11:00:52AM +0000, Andrew Cooper wrote:
> > > On 30/12/2015 05:25, Wen Congyang wrote:
> > > > On 12/30/2015 12:11 PM, Doug Goldstein wrote:
> > > > > On 12/29/15 8:39 PM, Wen Congyang wrote:
> > > > > > We may use non-root user to run qemu, and the qemu needs to
> > > > > > write
> > > > > > save file to /var/lib/xen. So we should allow all user to
> > > > > > create
> > > > > > a file under the directory /var/lib/xen
> > > > > > 
> > > > > > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> > > > > > ---
> > > > > >  tools/Makefile | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/tools/Makefile b/tools/Makefile
> > > > > > index 820ca40..402b417 100644
> > > > > > --- a/tools/Makefile
> > > > > > +++ b/tools/Makefile
> > > > > > @@ -60,7 +60,7 @@ build all: subdirs-all
> > > > > >  install: subdirs-install
> > > > > >  	$(INSTALL_DIR) -m 700 $(DESTDIR)$(XEN_DUMP_DIR)
> > > > > >  	$(INSTALL_DIR) $(DESTDIR)/var/log/xen
> > > > > > -	$(INSTALL_DIR) $(DESTDIR)/var/lib/xen
> > > > > > +	$(INSTALL_DIR) -m 777 $(DESTDIR)/var/lib/xen
> > > > > >  .PHONY: uninstall
> > > > > >  uninstall: D=$(DESTDIR)
> > > > > > 
> > > > > I could be wrong but this doesn't seem like something that you'd
> > > > > want to
> > > > > do given what's stored in there. Could you do something with
> > > > > permissions
> > > > > on sub-directories to achieve what you need?
> > > > > 
> > > > The save file's path is:
> > > > #define LIBXL_DEVICE_MODEL_SAVE_FILE "/var/lib/xen/qemu-save" /*
> > > > .$domid */
> > > > 
> > > > So all user must have write permission on the directory
> > > > /var/lib/xen/, otherwise,
> > > > the migration will fail.
> > > For now, I would avoid running qemu as a non-root user.  It doesn't
> > > gain you
> > > any meaninful security at present (at the expense of a warning which
> > > can't
> > > be turned off).
> > > 
> > > As to this bug, marking the directory 0777 is not an option, as save
> > > records
> > > necessarily contain sensitive data.
> > > 
> > > Longterm, (and already identified in one of the threads in the past),
> > > the
> > > best course of action is to switch away from having files, and
> > > passing file
> > > descriptors instead.  This is more flexible (currently libxl can't
> > > function
> > > on a read-only root filesystem), and would allow a privileged entity
> > > to open
> > > the file descriptor and pass it to a non-privileged entity to
> > > use.  This
> > > allows the non-privileged entity to function, and maintains security.
> > Wen,
> > 
> > Could you mention the use case for wanting to write files there?
> > Looking
> > at the patches you had sent for COLO and Remus they use an file
> > descriptor - so
> > what is the use-case here?
> 
> This is a bug in existing code.  It is not a COLO specific issue.
> 
> The current protocol for live migration requires Qemu to write its save
> file here.
> 
> Until this issue is resolved, live migration is inoperable with Qemu
> running as a non-root user.

Stefano, is this already on your list of issues to address?

In any case creating a world writeable directory is clearly a non-starter.
We might need the toolstack to create a directory with suitable permissions
until we can rework things to work with fds only.

Ian.


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

Re: [PATCH 5/5] Allow all user to create a file under the directory /var/lib/xen

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 4724 bytes --]

On Tue, 26 Jan 2016, Ian Campbell wrote:
> On Tue, 2016-01-26 at 00:00 +0000, Andrew Cooper wrote:
> > On 25/01/2016 20:36, Konrad Rzeszutek Wilk wrote:
> > > On Wed, Dec 30, 2015 at 11:00:52AM +0000, Andrew Cooper wrote:
> > > > On 30/12/2015 05:25, Wen Congyang wrote:
> > > > > On 12/30/2015 12:11 PM, Doug Goldstein wrote:
> > > > > > On 12/29/15 8:39 PM, Wen Congyang wrote:
> > > > > > > We may use non-root user to run qemu, and the qemu needs to
> > > > > > > write
> > > > > > > save file to /var/lib/xen. So we should allow all user to
> > > > > > > create
> > > > > > > a file under the directory /var/lib/xen
> > > > > > >
> > > > > > > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> > > > > > > ---
> > > > > > >  tools/Makefile | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/tools/Makefile b/tools/Makefile
> > > > > > > index 820ca40..402b417 100644
> > > > > > > --- a/tools/Makefile
> > > > > > > +++ b/tools/Makefile
> > > > > > > @@ -60,7 +60,7 @@ build all: subdirs-all
> > > > > > >  install: subdirs-install
> > > > > > >  	$(INSTALL_DIR) -m 700 $(DESTDIR)$(XEN_DUMP_DIR)
> > > > > > >  	$(INSTALL_DIR) $(DESTDIR)/var/log/xen
> > > > > > > -	$(INSTALL_DIR) $(DESTDIR)/var/lib/xen
> > > > > > > +	$(INSTALL_DIR) -m 777 $(DESTDIR)/var/lib/xen
> > > > > > >  .PHONY: uninstall
> > > > > > >  uninstall: D=$(DESTDIR)
> > > > > > >
> > > > > > I could be wrong but this doesn't seem like something that you'd
> > > > > > want to
> > > > > > do given what's stored in there. Could you do something with
> > > > > > permissions
> > > > > > on sub-directories to achieve what you need?
> > > > > >
> > > > > The save file's path is:
> > > > > #define LIBXL_DEVICE_MODEL_SAVE_FILE "/var/lib/xen/qemu-save" /*
> > > > > .$domid */
> > > > >
> > > > > So all user must have write permission on the directory
> > > > > /var/lib/xen/, otherwise,
> > > > > the migration will fail.
> > > > For now, I would avoid running qemu as a non-root user.  It doesn't
> > > > gain you
> > > > any meaninful security at present (at the expense of a warning which
> > > > can't
> > > > be turned off).
> > > >
> > > > As to this bug, marking the directory 0777 is not an option, as save
> > > > records
> > > > necessarily contain sensitive data.
> > > >
> > > > Longterm, (and already identified in one of the threads in the past),
> > > > the
> > > > best course of action is to switch away from having files, and
> > > > passing file
> > > > descriptors instead.  This is more flexible (currently libxl can't
> > > > function
> > > > on a read-only root filesystem), and would allow a privileged entity
> > > > to open
> > > > the file descriptor and pass it to a non-privileged entity to
> > > > use.  This
> > > > allows the non-privileged entity to function, and maintains security.
> > > Wen,
> > >
> > > Could you mention the use case for wanting to write files there?
> > > Looking
> > > at the patches you had sent for COLO and Remus they use an file
> > > descriptor - so
> > > what is the use-case here?
> >
> > This is a bug in existing code.  It is not a COLO specific issue.
> >
> > The current protocol for live migration requires Qemu to write its save
> > file here.
> >
> > Until this issue is resolved, live migration is inoperable with Qemu
> > running as a non-root user.
>
> Stefano, is this already on your list of issues to address?
>
> In any case creating a world writeable directory is clearly a non-starter.
> We might need the toolstack to create a directory with suitable permissions
> until we can rework things to work with fds only.

It is sufficient to create an empty save file, as returned by
libxl__device_model_savefile, with the right owner, at domain creation
time. Something like below:

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index a088d71..f908422 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1285,6 +1285,12 @@ end_search:
         if (user != NULL && strcmp(user, "root")) {
             flexarray_append(dm_args, "-runas");
             flexarray_append(dm_args, user);
+
+            const char *filename = libxl__device_model_savefile(gc, guest_domid);
+            int fd = open(filename, O_WRONLY|O_CREAT|O_NOCTTY|O_NONBLOCK, 0600);
+            struct passwd *pw = getpwnam(user);
+            fchown(fd, pw->pw_uid, pw->pw_gid);
+            close(fd);
         }
     }
     flexarray_append(dm_args, NULL);

This is another thing that would be easier to fix after Ian's privsep
series, because we'll have a better place for this code.

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

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

Re: [PATCH 5/5] Allow all user to create a file under the directory /var/lib/xen

[Ian Campbell]

On Tue, 2016-01-26 at 17:15 +0000, Stefano Stabellini wrote:
> It is sufficient to create an empty save file, as returned by
> libxl__device_model_savefile, with the right owner, at domain creation
> time. Something like below:
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index a088d71..f908422 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1285,6 +1285,12 @@ end_search:
>          if (user != NULL && strcmp(user, "root")) {
>              flexarray_append(dm_args, "-runas");
>              flexarray_append(dm_args, user);
> +
> +            const char *filename = libxl__device_model_savefile(gc, guest_domid);
> +            int fd = open(filename, O_WRONLY|O_CREAT|O_NOCTTY|O_NONBLOCK, 0600);
> +            struct passwd *pw = getpwnam(user);
> +            fchown(fd, pw->pw_uid, pw->pw_gid);
> +            close(fd);
>          }
>      }
>      flexarray_append(dm_args, NULL);
> 
> This is another thing that would be easier to fix after Ian's privsep
> series, because we'll have a better place for this code.

Right.

In the meantime I think we should update at least docs/misc/qemu-
deprivilege.txt and probably docs/man/xl.cfg.pod.5:device_model_user to
mention the shortcomings of using these options if they are going to break
core functionality such as migration.

Would you knock up a patch please?

At the same time, I wonder if docs/misc/qemu-deprivilege.txt ought to move
to docs/features/ ?

Ian.

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

Re: [PATCH 0/5] migration/remus: bug fix and cleanup

[Andrew Cooper]

On 30/12/2015 01:39, Wen Congyang wrote:
>
> Wen Congyang (5):
>    remus: don't call stream_continue() when doing failover
>    remus: don't write xenstore data if it fails
>    tools/libxc: don't send end record if remus fails
>    tools/libxl: remove unused function libxl__domain_save_device_model()
>    Allow all user to create a file under the directory /var/lib/xen
>
>   tools/Makefile                   |  2 +-
>   tools/libxc/xc_sr_save.c         |  2 +-
>   tools/libxl/libxl_dom.c          | 91 ----------------------------------------
>   tools/libxl/libxl_internal.h     |  3 --
>   tools/libxl/libxl_stream_read.c  | 18 +++++---
>   tools/libxl/libxl_stream_write.c |  8 +++-
>   6 files changed, 21 insertions(+), 103 deletions(-)

Just as a reminder, you need to CC the toolstack maintainers on all 
patches in this series.

~Andrew

Re: [PATCH 0/5] migration/remus: bug fix and cleanup

[Wen Congyang]

On 12/30/2015 06:38 PM, Andrew Cooper wrote:
> On 30/12/2015 01:39, Wen Congyang wrote:
>>
>> Wen Congyang (5):
>>    remus: don't call stream_continue() when doing failover
>>    remus: don't write xenstore data if it fails
>>    tools/libxc: don't send end record if remus fails
>>    tools/libxl: remove unused function libxl__domain_save_device_model()
>>    Allow all user to create a file under the directory /var/lib/xen
>>
>>   tools/Makefile                   |  2 +-
>>   tools/libxc/xc_sr_save.c         |  2 +-
>>   tools/libxl/libxl_dom.c          | 91 ----------------------------------------
>>   tools/libxl/libxl_internal.h     |  3 --
>>   tools/libxl/libxl_stream_read.c  | 18 +++++---
>>   tools/libxl/libxl_stream_write.c |  8 +++-
>>   6 files changed, 21 insertions(+), 103 deletions(-)
> 
> Just as a reminder, you need to CC the toolstack maintainers on all patches in this series.

OK, I will CC them in the next version.

Thanks
Wen Congyang

> 
> ~Andrew
> 
> 
> .
>