Xen devel - Nov 2011 - [PATCH] Prevent xl save from segfaulting when control/shutdown key is removed

# HG changeset patch
# User Paul Durrant <paul.durrant@citrix.com>
# Date 1322041530 0
# Node ID 3341e3e990568f459ae984cd9d2cac2d546eaa4e
# Parent  0a0c02a616768bfab16c072788cb76be1893c37f
Prevent xl save from segfaulting when control/shutdown key is removed

To acknowledge the tools'' setting of control/shutdown it is normal for
PV drivers
to rm the key. This leads to libxl__xs_read() returning NULL and thus a
subsequent
strcmp on the return value will cause a segfault.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

diff -r 0a0c02a61676 -r 3341e3e99056 tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c	Mon Nov 21 21:28:34 2011 +0000
+++ b/tools/libxl/libxl_dom.c	Wed Nov 23 09:45:30 2011 +0000
@@ -444,6 +444,7 @@ static int libxl__domain_suspend_common_
             usleep(100000);
 
             state = libxl__xs_read(si->gc, XBT_NULL, path);
+            if (!state) state = "";
 
             watchdog--;
         }
@@ -463,6 +464,7 @@ static int libxl__domain_suspend_common_
             t = xs_transaction_start(ctx->xsh);
 
             state = libxl__xs_read(si->gc, t, path);
+            if (!state) state = "";
 
             if (!strcmp(state, "suspend"))
                 libxl__xs_write(si->gc, t, path, "");

On Wed, 2011-11-23 at 09:45 +0000, Paul Durrant wrote:
> # HG changeset patch
> # User Paul Durrant <paul.durrant@citrix.com>
> # Date 1322041530 0
> # Node ID 3341e3e990568f459ae984cd9d2cac2d546eaa4e
> # Parent  0a0c02a616768bfab16c072788cb76be1893c37f
> Prevent xl save from segfaulting when control/shutdown key is removed
> 
> To acknowledge the tools'' setting of control/shutdown it is normal
for PV drivers
> to rm the key. This leads to libxl__xs_read() returning NULL and thus a
subsequent
> strcmp on the return value will cause a segfault.
The Linux PV drivers actually write "" rather than removing the key
which is how this didn''t get spotted already. We should be robust to PV
drivers which remove it as well.

At start of day .../control is created ro (to the guest)
whereas .../control/shutdown is created rw. Can you confirm that a
second operation still succeeds if the guest rm''s the node on the first
action?

My concern is that while the first time the round the node will be rw
the second time round the write will actually re-create the node
(without setting the permissions) which might result in the node being
ro for the guest (xenstore perms confuse me, but I think new nodes
inherit the parent permissions).

That''s assuming there''s any chance of a second operation.
I''m thinking
of a wedged reboot followed by an attempt to shutdown instead or
something like that. Perhaps in practice that wouldn''t work anyway.

Apart form the above this change improves the robustness of the code so:
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>


> 
> diff -r 0a0c02a61676 -r 3341e3e99056 tools/libxl/libxl_dom.c
> --- a/tools/libxl/libxl_dom.c	Mon Nov 21 21:28:34 2011 +0000
> +++ b/tools/libxl/libxl_dom.c	Wed Nov 23 09:45:30 2011 +0000
> @@ -444,6 +444,7 @@ static int libxl__domain_suspend_common_
>              usleep(100000);
>  
>              state = libxl__xs_read(si->gc, XBT_NULL, path);
> +            if (!state) state = "";
>  
>              watchdog--;
>          }
> @@ -463,6 +464,7 @@ static int libxl__domain_suspend_common_
>              t = xs_transaction_start(ctx->xsh);
>  
>              state = libxl__xs_read(si->gc, t, path);
> +            if (!state) state = "";
>  
>              if (!strcmp(state, "suspend"))
>                  libxl__xs_write(si->gc, t, path, "");
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

> -----Original Message-----
> From: Ian Campbell
> Sent: 23 November 2011 10:13
> To: Paul Durrant
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH] Prevent xl save from segfaulting
> when control/shutdown key is removed
> 
> On Wed, 2011-11-23 at 09:45 +0000, Paul Durrant wrote:
> > # HG changeset patch
> > # User Paul Durrant <paul.durrant@citrix.com> # Date 1322041530
0
> #
> > Node ID 3341e3e990568f459ae984cd9d2cac2d546eaa4e
> > # Parent  0a0c02a616768bfab16c072788cb76be1893c37f
> > Prevent xl save from segfaulting when control/shutdown key is
> removed
> >
> > To acknowledge the tools'' setting of control/shutdown it is
normal
> for
> > PV drivers to rm the key. This leads to libxl__xs_read() returning
> > NULL and thus a subsequent strcmp on the return value will cause a
> segfault.
> 
> The Linux PV drivers actually write "" rather than removing the
key
> which is how this didn''t get spotted already. We should be robust
to
> PV drivers which remove it as well.
> 
> At start of day .../control is created ro (to the guest) whereas
> .../control/shutdown is created rw. Can you confirm that a second
> operation still succeeds if the guest rm''s the node on the first
> action?
> 
At the moment I can''t. I''m getting a complete VM wedge-up on
restore somewhere in the region of re-connecting to store. I''ll check
the subsequent suspend behaviour once I can get the Vm to actually come back.
BTW what is the reason for creating control ro to the guest? In XenServer we
allow the guest to write the control key to advertise feature-shutdown,
feature-suspend etc. so that the tools know what values of control/shutdown the
guest will respond to.

  Paul
> My concern is that while the first time the round the node will be
> rw the second time round the write will actually re-create the node
> (without setting the permissions) which might result in the node
> being ro for the guest (xenstore perms confuse me, but I think new
> nodes inherit the parent permissions).
> 
> That''s assuming there''s any chance of a second operation.
I''m
> thinking of a wedged reboot followed by an attempt to shutdown
> instead or something like that. Perhaps in practice that wouldn''t
> work anyway.
> 
> Apart form the above this change improves the robustness of the code
> so:
> 
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> 
> 
> >
> > diff -r 0a0c02a61676 -r 3341e3e99056 tools/libxl/libxl_dom.c
> > --- a/tools/libxl/libxl_dom.c	Mon Nov 21 21:28:34 2011 +0000
> > +++ b/tools/libxl/libxl_dom.c	Wed Nov 23 09:45:30 2011 +0000
> > @@ -444,6 +444,7 @@ static int libxl__domain_suspend_common_
> >              usleep(100000);
> >
> >              state = libxl__xs_read(si->gc, XBT_NULL, path);
> > +            if (!state) state = "";
> >
> >              watchdog--;
> >          }
> > @@ -463,6 +464,7 @@ static int libxl__domain_suspend_common_
> >              t = xs_transaction_start(ctx->xsh);
> >
> >              state = libxl__xs_read(si->gc, t, path);
> > +            if (!state) state = "";
> >
> >              if (!strcmp(state, "suspend"))
> >                  libxl__xs_write(si->gc, t, path, "");
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel
>

On Wed, 2011-11-23 at 11:19 +0000, Paul Durrant wrote:
> what is the reason for creating control ro to the guest?
In general libxl prefers to whitelist paths which the guest can write
too, just to prevent a complete free for all, keep things somewhat under
control and to help avoid situations where tools might inadvertently
rely on a guest-writeable key in an unsafe way..
>  In XenServer we allow the guest to write the control key to advertise
> feature-shutdown, feature-suspend etc. so that the tools know what
> values of control/shutdown the guest will respond to.
The libxl way would be to create these at build time (perhaps empty)
with the appropriate permissions.

It''s not clear how that functionality can be added in a way which is
compatible with existing guests though, e.g. no Linux guest writes those
but many can be suspended etc.

Ian.
> 
>   Paul
> 
> > My concern is that while the first time the round the node will be
> > rw the second time round the write will actually re-create the node
> > (without setting the permissions) which might result in the node
> > being ro for the guest (xenstore perms confuse me, but I think new
> > nodes inherit the parent permissions).
> > 
> > That''s assuming there''s any chance of a second
operation. I''m
> > thinking of a wedged reboot followed by an attempt to shutdown
> > instead or something like that. Perhaps in practice that
wouldn''t
> > work anyway.
> > 
> > Apart form the above this change improves the robustness of the code
> > so:
> > 
> > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > 
> > 
> > >
> > > diff -r 0a0c02a61676 -r 3341e3e99056 tools/libxl/libxl_dom.c
> > > --- a/tools/libxl/libxl_dom.c	Mon Nov 21 21:28:34 2011 +0000
> > > +++ b/tools/libxl/libxl_dom.c	Wed Nov 23 09:45:30 2011 +0000
> > > @@ -444,6 +444,7 @@ static int libxl__domain_suspend_common_
> > >              usleep(100000);
> > >
> > >              state = libxl__xs_read(si->gc, XBT_NULL, path);
> > > +            if (!state) state = "";
> > >
> > >              watchdog--;
> > >          }
> > > @@ -463,6 +464,7 @@ static int libxl__domain_suspend_common_
> > >              t = xs_transaction_start(ctx->xsh);
> > >
> > >              state = libxl__xs_read(si->gc, t, path);
> > > +            if (!state) state = "";
> > >
> > >              if (!strcmp(state, "suspend"))
> > >                  libxl__xs_write(si->gc, t, path,
"");
> > >
> > > _______________________________________________
> > > Xen-devel mailing list
> > > Xen-devel@lists.xensource.com
> > > http://lists.xensource.com/xen-devel
> > 
>

Paul Durrant writes ("[Xen-devel] [PATCH] Prevent xl save from segfaulting
when control/shutdown key is removed"):
> Prevent xl save from segfaulting when control/shutdown key is removed
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

> -----Original Message-----
> From: Ian Campbell
> Sent: 23 November 2011 11:24
> To: Paul Durrant
> Cc: xen-devel@lists.xensource.com
> Subject: RE: [Xen-devel] [PATCH] Prevent xl save from segfaulting
> when control/shutdown key is removed
> 
> On Wed, 2011-11-23 at 11:19 +0000, Paul Durrant wrote:
> > what is the reason for creating control ro to the guest?
> 
> In general libxl prefers to whitelist paths which the guest can
> write too, just to prevent a complete free for all, keep things
> somewhat under control and to help avoid situations where tools
> might inadvertently rely on a guest-writeable key in an unsafe way..
> 
> >  In XenServer we allow the guest to write the control key to
> advertise
> > feature-shutdown, feature-suspend etc. so that the tools know what
> > values of control/shutdown the guest will respond to.
> 
> The libxl way would be to create these at build time (perhaps empty)
> with the appropriate permissions.
> 
> It''s not clear how that functionality can be added in a way which
is
> compatible with existing guests though, e.g. no Linux guest writes
> those but many can be suspended etc.
> 
So the simple solution, for compatibility''s sake, is to make control rw
isn''t it?

  Paul

On Wed, 2011-11-23 at 12:59 +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Ian Campbell
> > Sent: 23 November 2011 11:24
> > To: Paul Durrant
> > Cc: xen-devel@lists.xensource.com
> > Subject: RE: [Xen-devel] [PATCH] Prevent xl save from segfaulting
> > when control/shutdown key is removed
> > 
> > On Wed, 2011-11-23 at 11:19 +0000, Paul Durrant wrote:
> > > what is the reason for creating control ro to the guest?
> > 
> > In general libxl prefers to whitelist paths which the guest can
> > write too, just to prevent a complete free for all, keep things
> > somewhat under control and to help avoid situations where tools
> > might inadvertently rely on a guest-writeable key in an unsafe way..
> > 
> > >  In XenServer we allow the guest to write the control key to
> > advertise
> > > feature-shutdown, feature-suspend etc. so that the tools know
what
> > > values of control/shutdown the guest will respond to.
> > 
> > The libxl way would be to create these at build time (perhaps empty)
> > with the appropriate permissions.
> > 
> > It''s not clear how that functionality can be added in a way
which is
> > compatible with existing guests though, e.g. no Linux guest writes
> > those but many can be suspended etc.
> > 
> 
> So the simple solution, for compatibility''s sake, is to make
control rw isn''t it?
The problem I''m thinking of exists even if control is rw.

Given an empty control directory how do you know if a guest supports
suspend or not, given that most existing guests which do support suspend
do not write any key there?

Ian.

True. I was conflating the key rm issue with the fact that the XenServer Windows
PV guest agent gets unhappy because it can''t advertise its features
(which I hadn''t mentioned before).

I suppose that libxl will just have to continue to speculatively write
control/shutdown and timeout if it''s not NUL-ed or rm-ed as it does
now.

  Paul
> -----Original Message-----
> From: Ian Campbell
> Sent: 23 November 2011 13:05
> To: Paul Durrant
> Cc: xen-devel@lists.xensource.com
> Subject: RE: [Xen-devel] [PATCH] Prevent xl save from segfaulting
> when control/shutdown key is removed
> 
> On Wed, 2011-11-23 at 12:59 +0000, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Ian Campbell
> > > Sent: 23 November 2011 11:24
> > > To: Paul Durrant
> > > Cc: xen-devel@lists.xensource.com
> > > Subject: RE: [Xen-devel] [PATCH] Prevent xl save from
> segfaulting
> > > when control/shutdown key is removed
> > >
> > > On Wed, 2011-11-23 at 11:19 +0000, Paul Durrant wrote:
> > > > what is the reason for creating control ro to the guest?
> > >
> > > In general libxl prefers to whitelist paths which the guest can
> > > write too, just to prevent a complete free for all, keep things
> > > somewhat under control and to help avoid situations where tools
> > > might inadvertently rely on a guest-writeable key in an unsafe
> way..
> > >
> > > >  In XenServer we allow the guest to write the control key to
> > > advertise
> > > > feature-shutdown, feature-suspend etc. so that the tools
know
> what
> > > > values of control/shutdown the guest will respond to.
> > >
> > > The libxl way would be to create these at build time (perhaps
> empty)
> > > with the appropriate permissions.
> > >
> > > It''s not clear how that functionality can be added in a
way
> which is
> > > compatible with existing guests though, e.g. no Linux guest
> writes
> > > those but many can be suspended etc.
> > >
> >
> > So the simple solution, for compatibility''s sake, is to make
> control rw isn''t it?
> 
> The problem I''m thinking of exists even if control is rw.
> 
> Given an empty control directory how do you know if a guest supports
> suspend or not, given that most existing guests which do support
> suspend do not write any key there?
> 
> Ian.

On Wed, 2011-11-23 at 13:12 +0000, Paul Durrant wrote:
> True. I was conflating the key rm issue with the fact that the
> XenServer Windows PV guest agent gets unhappy because it can''t
> advertise its features (which I hadn''t mentioned before).
> 
> I suppose that libxl will just have to continue to speculatively write
> control/shutdown and timeout if it''s not NUL-ed or rm-ed as it
does
> now.
I suppose one option might be a "feature-will-write-features" flag. If
it is not written to true then assume everything is supported, if it
does get set to true then look for the flags and follow them. However if
a protocol is going to be invented then it should be discussed here and
documented etc.

Ian.
> 
>   Paul
> 
> > -----Original Message-----
> > From: Ian Campbell
> > Sent: 23 November 2011 13:05
> > To: Paul Durrant
> > Cc: xen-devel@lists.xensource.com
> > Subject: RE: [Xen-devel] [PATCH] Prevent xl save from segfaulting
> > when control/shutdown key is removed
> > 
> > On Wed, 2011-11-23 at 12:59 +0000, Paul Durrant wrote:
> > > > -----Original Message-----
> > > > From: Ian Campbell
> > > > Sent: 23 November 2011 11:24
> > > > To: Paul Durrant
> > > > Cc: xen-devel@lists.xensource.com
> > > > Subject: RE: [Xen-devel] [PATCH] Prevent xl save from
> > segfaulting
> > > > when control/shutdown key is removed
> > > >
> > > > On Wed, 2011-11-23 at 11:19 +0000, Paul Durrant wrote:
> > > > > what is the reason for creating control ro to the
guest?
> > > >
> > > > In general libxl prefers to whitelist paths which the guest
can
> > > > write too, just to prevent a complete free for all, keep
things
> > > > somewhat under control and to help avoid situations where
tools
> > > > might inadvertently rely on a guest-writeable key in an
unsafe
> > way..
> > > >
> > > > >  In XenServer we allow the guest to write the control
key to
> > > > advertise
> > > > > feature-shutdown, feature-suspend etc. so that the
tools know
> > what
> > > > > values of control/shutdown the guest will respond to.
> > > >
> > > > The libxl way would be to create these at build time
(perhaps
> > empty)
> > > > with the appropriate permissions.
> > > >
> > > > It''s not clear how that functionality can be added
in a way
> > which is
> > > > compatible with existing guests though, e.g. no Linux guest
> > writes
> > > > those but many can be suspended etc.
> > > >
> > >
> > > So the simple solution, for compatibility''s sake, is to
make
> > control rw isn''t it?
> > 
> > The problem I''m thinking of exists even if control is rw.
> > 
> > Given an empty control directory how do you know if a guest supports
> > suspend or not, given that most existing guests which do support
> > suspend do not write any key there?
> > 
> > Ian.
>