[PATCH v2] libxl: Reset toolstack_save file position in libxl

[PATCH v2] libxl: Reset toolstack_save file position in libxl

[Jason Andryuk]

toolstack_save data is written to a temporary file in libxl and read
back in libxl-save-helper.  The file position must be reset prior to
reading the file, which is done in libxl-save-helper with lseek.

lseek is unsupported for pipes and sockets, so a wrapper passing such an
fd to libxl-save-helper fails the lseek.  Moving the lseek to libxl
avoids the error, allowing the save to continue.

Signed-off-by: Jason Andryuk <andryuk@aero.org>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v2:
Use single line comment
---
 tools/libxl/libxl_save_callout.c | 4 ++++
 tools/libxl/libxl_save_helper.c  | 5 ++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_save_callout.c b/tools/libxl/libxl_save_callout.c
index 6e45b2f..1bfedfb 100644
--- a/tools/libxl/libxl_save_callout.c
+++ b/tools/libxl/libxl_save_callout.c
@@ -105,6 +105,10 @@ void libxl__xc_domain_save(libxl__egc *egc, libxl__domain_suspend_state *dss,
                                 toolstack_data_buf, toolstack_data_len,
                                 "toolstack data tmpfile", 0);
         if (r) { rc = ERROR_FAIL; goto out; }
+
+        /* file position must be reset before passing to libxl-save-helper. */
+        r = lseek(toolstack_data_fd, 0, SEEK_SET);
+        if (r) { rc = ERROR_FAIL; goto out; }
     }
 
     const unsigned long argnums[] = {
diff --git a/tools/libxl/libxl_save_helper.c b/tools/libxl/libxl_save_helper.c
index 880565e..20ca13e 100644
--- a/tools/libxl/libxl_save_helper.c
+++ b/tools/libxl/libxl_save_helper.c
@@ -169,10 +169,9 @@ static uint32_t toolstack_save_len;
 static int toolstack_save_cb(uint32_t domid, uint8_t **buf,
                              uint32_t *len, void *data)
 {
-    assert(toolstack_save_fd > 0);
+    int r;
 
-    int r = lseek(toolstack_save_fd, 0, SEEK_SET);
-    if (r) fail(errno,"rewind toolstack data tmpfile");
+    assert(toolstack_save_fd > 0);
 
     *buf = xmalloc(toolstack_save_len);
     r = read_exactly(toolstack_save_fd, *buf, toolstack_save_len);
-- 
1.8.3.1

Re: [PATCH v2] libxl: Reset toolstack_save file position in libxl

[Andrew Cooper]

On 19/05/2014 19:36, Jason Andryuk wrote:
> toolstack_save data is written to a temporary file in libxl and read
> back in libxl-save-helper.  The file position must be reset prior to
> reading the file, which is done in libxl-save-helper with lseek.
>
> lseek is unsupported for pipes and sockets, so a wrapper passing such an
> fd to libxl-save-helper fails the lseek.  Moving the lseek to libxl
> avoids the error, allowing the save to continue.
>
> Signed-off-by: Jason Andryuk <andryuk@aero.org>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Ian: As noted previously, this should be a backport candidate.

> ---
> v2:
> Use single line comment
> ---
>  tools/libxl/libxl_save_callout.c | 4 ++++
>  tools/libxl/libxl_save_helper.c  | 5 ++---
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/tools/libxl/libxl_save_callout.c b/tools/libxl/libxl_save_callout.c
> index 6e45b2f..1bfedfb 100644
> --- a/tools/libxl/libxl_save_callout.c
> +++ b/tools/libxl/libxl_save_callout.c
> @@ -105,6 +105,10 @@ void libxl__xc_domain_save(libxl__egc *egc, libxl__domain_suspend_state *dss,
>                                  toolstack_data_buf, toolstack_data_len,
>                                  "toolstack data tmpfile", 0);
>          if (r) { rc = ERROR_FAIL; goto out; }
> +
> +        /* file position must be reset before passing to libxl-save-helper. */
> +        r = lseek(toolstack_data_fd, 0, SEEK_SET);
> +        if (r) { rc = ERROR_FAIL; goto out; }
>      }
>  
>      const unsigned long argnums[] = {
> diff --git a/tools/libxl/libxl_save_helper.c b/tools/libxl/libxl_save_helper.c
> index 880565e..20ca13e 100644
> --- a/tools/libxl/libxl_save_helper.c
> +++ b/tools/libxl/libxl_save_helper.c
> @@ -169,10 +169,9 @@ static uint32_t toolstack_save_len;
>  static int toolstack_save_cb(uint32_t domid, uint8_t **buf,
>                               uint32_t *len, void *data)
>  {
> -    assert(toolstack_save_fd > 0);
> +    int r;
>  
> -    int r = lseek(toolstack_save_fd, 0, SEEK_SET);
> -    if (r) fail(errno,"rewind toolstack data tmpfile");
> +    assert(toolstack_save_fd > 0);
>  
>      *buf = xmalloc(toolstack_save_len);
>      r = read_exactly(toolstack_save_fd, *buf, toolstack_save_len);

Re: [PATCH v2] libxl: Reset toolstack_save file position in libxl

[Ian Jackson]

Andrew Cooper writes ("Re: [PATCH v2] libxl: Reset toolstack_save file position in libxl"):
> On 19/05/2014 19:36, Jason Andryuk wrote:
> > toolstack_save data is written to a temporary file in libxl and read
> > back in libxl-save-helper.  The file position must be reset prior to
> > reading the file, which is done in libxl-save-helper with lseek.
> >
> > lseek is unsupported for pipes and sockets, so a wrapper passing such an
> > fd to libxl-save-helper fails the lseek.  Moving the lseek to libxl
> > avoids the error, allowing the save to continue.
> >
> > Signed-off-by: Jason Andryuk <andryuk@aero.org>
> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Ian: As noted previously, this should be a backport candidate.

We normally do backports only for bugs (and not even all bugs).  I'm
afraid that I don't see how the current arrangements are a bug.

See my other mail.  Perhaps the answer to that will explain to me how
the current are a bug, rather than simply less convenient and flexible
than they could be.

Thanks,
Ian.

Re: [PATCH v2] libxl: Reset toolstack_save file position in libxl

[Andrew Cooper]

On 21/05/14 17:04, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH v2] libxl: Reset toolstack_save file position in libxl"):
>> On 19/05/2014 19:36, Jason Andryuk wrote:
>>> toolstack_save data is written to a temporary file in libxl and read
>>> back in libxl-save-helper.  The file position must be reset prior to
>>> reading the file, which is done in libxl-save-helper with lseek.
>>>
>>> lseek is unsupported for pipes and sockets, so a wrapper passing such an
>>> fd to libxl-save-helper fails the lseek.  Moving the lseek to libxl
>>> avoids the error, allowing the save to continue.
>>>
>>> Signed-off-by: Jason Andryuk <andryuk@aero.org>
>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Ian: As noted previously, this should be a backport candidate.
> We normally do backports only for bugs (and not even all bugs).  I'm
> afraid that I don't see how the current arrangements are a bug.
>
> See my other mail.  Perhaps the answer to that will explain to me how
> the current are a bug, rather than simply less convenient and flexible
> than they could be.
>
> Thanks,
> Ian.

It is a layering violation.  libxl-save-helper cannot assume that the fd
is a file, so shouldn't seek on it.

libxl however should prepare a "socket-like" fd to the save-helper,
which means rewinding the file itself.


As indicated, "toolstack_data" is heading very much in the direction of
/dev/null with the migration v2 patches, as it itself is a gross
layering violation. If you don't deem this worthy for backport, its not
the end of the world.

~Andrew

Re: [PATCH v2] libxl: Reset toolstack_save file position in libxl

[Ian Jackson]

Jason Andryuk writes ("[PATCH v2] libxl: Reset toolstack_save file position in libxl"):
> toolstack_save data is written to a temporary file in libxl and read
> back in libxl-save-helper.  The file position must be reset prior to
> reading the file, which is done in libxl-save-helper with lseek.
> 
> lseek is unsupported for pipes and sockets, so a wrapper passing such an
> fd to libxl-save-helper fails the lseek.  Moving the lseek to libxl
> avoids the error, allowing the save to continue.

I don't object to this in principle, and arguably it's wrong that this
functionality should be in the save helper rather than libxl proper
(since the save helper is supposed to be an as-thin-as-possible
wrapper around the libxc functions).  So TBH I'm inclined to take
this change on those grounds.

But I'm curious as to what kind of wrapper you have devised, and for
what purpose.  Do you mean a wrapper program for libxl-save-helper ?
Which presumably interposes a pipe for the toolstack data fd ?

Thanks,
Ian.

Re: [PATCH v2] libxl: Reset toolstack_save file position in libxl

[Jason Andryuk]

On 5/21/2014 12:02 PM, Ian Jackson wrote:
> Jason Andryuk writes ("[PATCH v2] libxl: Reset toolstack_save file position in libxl"):
>> toolstack_save data is written to a temporary file in libxl and read
>> back in libxl-save-helper.  The file position must be reset prior to
>> reading the file, which is done in libxl-save-helper with lseek.
>>
>> lseek is unsupported for pipes and sockets, so a wrapper passing such an
>> fd to libxl-save-helper fails the lseek.  Moving the lseek to libxl
>> avoids the error, allowing the save to continue.
> 
> I don't object to this in principle, and arguably it's wrong that this
> functionality should be in the save helper rather than libxl proper
> (since the save helper is supposed to be an as-thin-as-possible
> wrapper around the libxc functions).  So TBH I'm inclined to take
> this change on those grounds.
> 
> But I'm curious as to what kind of wrapper you have devised, and for
> what purpose.  Do you mean a wrapper program for libxl-save-helper ?
> Which presumably interposes a pipe for the toolstack data fd ?

Using XSM Flask and Domain Builder [1], the hypervisor can protect domU memory from control domains.  The wrapper spawns a migrator domain to run libxl-save-helper.  This migrator domain has the XSM permission to access the domU memory and encrypts the data stream to continue protecting the domU.

The wrapper in the control domain plumbs stdin, stdout, io_fd, and toolstack_save_fd through a vchan to the migrator domain.  Inside the migrator domain, the vchan data streams are passed through pipes to libxl-save-helper.  (An earlier prototype used vifs and a socket).  The migrator domain's libxl-save-helper return value is passed to control domain, where the wrapper cleans up and then exits with the aforementioned return value.

Yes, it's an atypical setup.  The patch doesn't change co-located libxl/libxl-save-helper, but it does allow the possibility described above.

Regards,

Jason

[1] http://lists.xen.org/archives/html/xen-devel/2014-03/msg00320.html

Re: [PATCH v2] libxl: Reset toolstack_save file position in libxl

[Ian Jackson]

Jason Andryuk writes ("Re: [PATCH v2] libxl: Reset toolstack_save file position in libxl"):
> On 5/21/2014 12:02 PM, Ian Jackson wrote:
> > But I'm curious as to what kind of wrapper you have devised, and for
> > what purpose.  Do you mean a wrapper program for libxl-save-helper ?
> > Which presumably interposes a pipe for the toolstack data fd ?
> 
> Using XSM Flask and Domain Builder [1], the hypervisor can protect
> domU memory from control domains.  The wrapper spawns a migrator
> domain to run libxl-save-helper.  This migrator domain has the XSM
> permission to access the domU memory and encrypts the data stream to
> continue protecting the domU.

How exciting.  Well, excellent.  Thanks for the explanation.

> Yes, it's an atypical setup.  The patch doesn't change co-located
> libxl/libxl-save-helper, but it does allow the possibility described
> above.

Right.  I'll apply the patch.  But I don't think this merits a
backport.

Thanks,
Ian.