Re: [Xen-devel] [PATCH v2 2/2] libxl: make creation of xenstore 'suspend event channel' node optional...

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

pdurrant@xxxxxxxx writes ("[PATCH v2 2/2] libxl: make creation of xenstore 
'suspend event channel' node optional..."):
> From: Paul Durrant <pdurrant@xxxxxxxxxx>
> 
> ... and make the top level 'device' node in xenstore writable by the
> guest

Sorry for taking a long time to review this.  Thanks for your
proposal.

> The purpose and semantics of the suspend event channel node are explained
> in xenstore-paths.pandoc [1]. It was originally introduced in xend by
> commit 17636f47a474 "Teach xc_save to use event-channel-based domain
> suspend if available.". Note that, because, the top-level frontend
> 'device' node was created writable by the guest in xend, there was no
> need to explicitly create the 'suspend-event-channel' node as writable
> node.
> 
> However, libxl creates the 'device' node as read-only by the guest and so
> explicit creation of the 'suspend-event-channel' node is necessary to make
> it usable. This unfortunately has the side-effect of making some old
> Windows PV drivers [2] cease to function. This is because they scan the top
> level 'device' node, find the 'suspend' node and expect it to contain the
> usual sub-nodes describing a PV frontend. When this is found not to be the
> case, enumeration ceases and (because the 'suspend' node is observed before
> the 'vbd' node) no system disk is enumerated. Windows will then crash with
> bugcheck code 0x7B.

This is quite a thorny problem.

I am uncomfortable with making ~/device writeable by the guest.  From
what you say it is necessary for at least these guests but I worry
that there might be something out there somewhere (maybe not even in
our tree) which trusts it too much.  (libxl used to be in this
category, before XSA-175/178, so this is sadly not a theoretical
concern.)  It's true that xend apparently make this writeable but
we have been making it readonly for a while now and maybe people
read xenstore-ls to see, or just didn't care...

I'm not sure how we could conduct an audit to find problems.  It seems
like that would be hard to do thoroughly (and a disproportionate
effort).  But we could at least
  - make this path writeable only as a bug compatibility feature,
     ie when needed to support this old guest
  - make sure we document it clearly in xenstore-paths as that
     is the arch reference that people will use when coding
  - document the fact that there may be security implications
     of setting thsi compat option

> This patch adds a boolean 'suspend_event_channel' field into
> libxl_create_info to control whether the xenstore node is created and a
> similarly named option in xl.cfg which, for compatibility with previous
> libxl behaviour, defaults to true. It also makes the top level device node
> writable, as xend did, and updates xenstore-paths.pandoc to say that the
> suspend event channel node may not exist and that the guest may create it
> if it does not exist.

So my inclination is to ask you to rework this patch so that:

  - the name of the config option more clearly indicates its status
     as a bug compat workaround
  - the ~/device writeability is controlled by the same compat flag,
     with corresponding update to the docs for the compat flag
  - adding an entry for the top-level ~/device to
     xenstore-paths.pandoc

But I am open to other approaches.

> NOTE: While adding the new LIBXL_HAVE_CREATEINFO_SUSPEND_EVENT_CHANNEL
>       definition into libxl.h, this patch corrects the previous stanza
>       which erroneously implies libxl_domain_create_info is a function.

Normally we like things broken out into their own patches but this one
is trivial I won't insist on that.

Ian.

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