[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 2/2] libxl: make creation of xenstore 'suspend event channel' node optional...
> -----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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |