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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Ian Jackson <ian.jackson@xxxxxxxxxx>
> Sent: 17 March 2020 16:51
> To: Paul Durrant <paul@xxxxxxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Wei Liu <wl@xxxxxxx>; Andrew Cooper 
> <Andrew.Cooper3@xxxxxxxxxx>;
> George Dunlap <George.Dunlap@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; 
> Julien Grall
> <julien@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Anthony Perard
> <anthony.perard@xxxxxxxxxx>
> Subject: Re: [PATCH v2 2/2] libxl: make creation of xenstore 'suspend event 
> channel' node optional...
> 
> 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.
>

Indeed.

 
> 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...
> 

That's true. I imagine most things don't care but there is a risk they might.

> 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).

Impossible really. We don't have code for all frontends :-(

>  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

I'm ok with that approach.

> 
> > 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

So, naming-wise... 'xend_compat', or is that too vague?

>   - 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.
> 

That all sounds fine.

> > 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.

OK. It did seem too trivial to break out.

  Paul

> 
> Ian.


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