From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH v1] Add build-id to XENVER hypercall. Date: Thu, 29 Oct 2015 15:47:07 -0400 Message-ID: <20151029194707.GA12231@x230.dumpdata.com> References: <1444359390-14153-1-git-send-email-konrad.wilk@oracle.com> <5617943C02000078000A99B8@prv-mh.provo.novell.com> <5617AFEE.4090908@citrix.com> <20151009132542.GH4709@l.oracle.com> <5617F5D902000078000A9CD1@prv-mh.provo.novell.com> <20151028154241.GA22160@l.oracle.com> <20151028190041.GC18498@l.oracle.com> <5631ED0D02000078000AFBC0@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZrtAP-0006zc-EH for xen-devel@lists.xenproject.org; Thu, 29 Oct 2015 19:47:21 +0000 Content-Disposition: inline In-Reply-To: <5631ED0D02000078000AFBC0@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: wei.liu2@citrix.com, ian.campbell@citrix.com, Andrew Cooper , ian.jackson@eu.citrix.com, mpohlack@amazon.de, xen-devel@lists.xenproject.org, dgdegra@tycho.nsa.gov List-Id: xen-devel@lists.xenproject.org On Thu, Oct 29, 2015 at 02:55:25AM -0600, Jan Beulich wrote: > >>> On 28.10.15 at 20:00, wrote: > > On Wed, Oct 28, 2015 at 11:42:41AM -0400, Konrad Rzeszutek Wilk wrote: > >> Perhaps an another option would be to return success and fill out the > >> value with an empty string? > >> > >> That actually sounds nicer. > > I disagree. You still change the ABI this way, the more that ... > > > Like this: > > > > From f5672c4b72361132798c0ec4bd124c9ddc80bd44 Mon Sep 17 00:00:00 2001 > > From: Konrad Rzeszutek Wilk > > Date: Mon, 28 Sep 2015 09:00:58 -0400 > > Subject: [PATCH] xsm/libxl/xen_version: Add XSM for the xen_version > > hypercall. > > > > All of XENVER_* have now an XSM check. > > > > The XENVER_[compile_info|changeset|commandline] are now > > guarded by an XSM check for priviliged domains. > > ... this matches what the patch does only in the dummy case (the > full policy case may yield any kind of behavior). > > Nevertheless a couple of comments on the patch itself: > > > --- a/tools/libxl/libxl.c > > +++ b/tools/libxl/libxl.c > > @@ -5288,6 +5288,8 @@ const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx) > > info->virt_start = u.p_parms.virt_start; > > > > info->pagesize = xc_version(ctx->xch, XENVER_pagesize, NULL); > > + if (!info->pagesize) /* No divide by zero! */ > > + info->pagesize = 1; > > I can't see any reason whatsoever to hide the page size from guests. > > > DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > > { > > + int empty_data = xsm_version_op(XSM_HOOK, cmd); > > The variable name kind of suggests it to have boolean meaning, and its > uses below don't help at all making clear that's not the case. Perhaps > better to make it bool_t and use !! above? > > > switch ( cmd ) > > { > > case XENVER_version: > > + if ( empty_data ) > > + return 0; > > return (xen_major_version() << 16) | xen_minor_version(); > > Another part I can't see a reason to hide. In fact, this may break > guests which adapt their behavior (use of certain hypercalls) > depending on hypervisor version. > > > @@ -277,6 +286,9 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > > .virt_start = HYPERVISOR_VIRT_START > > }; > > > > + if ( empty_data ) > > + params.virt_start = 0; > > This again may break guests (wanting to determine how much of the > address space to leave untouched). Our kernels use this (albeit with > proper error checking, so they wouldn't stop working, they just > would waste address space). > > > @@ -302,9 +315,14 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > > if ( copy_from_guest(&fi, arg, 1) ) > > return -EFAULT; > > > > + if ( empty_data ) > > + memset(&fi, 0, sizeof(fi)); > > + > > switch ( fi.submap_idx ) > > { > > case 0: > > + if ( empty_data ) > > + break; > > fi.submap = (1U << XENFEAT_memory_op_vnode_supported); > > if ( VM_ASSIST(d, pae_extended_cr3) ) > > fi.submap |= (1U << XENFEAT_pae_pgdir_above_4gb); > > This one, afaict, is _specifically_ meant to be available to everyone. OK, so we go back to that some of the subops should _not_ be behind an XSM check as they are meant to be available to everyone. Or rather - there is no point of an XSM check at all for those. > > > --- a/xen/include/xsm/dummy.h > > +++ b/xen/include/xsm/dummy.h > > @@ -720,4 +720,27 @@ static XSM_INLINE int xsm_pmu_op (XSM_DEFAULT_ARG struct domain *d, unsigned int > > } > > } > > > > +#include > > +static XSM_INLINE int xsm_version_op (XSM_DEFAULT_ARG uint32_t op) > > +{ > > + XSM_ASSERT_ACTION(XSM_HOOK); > > + switch ( op ) > > + { > > + case XENVER_compile_info: > > + case XENVER_changeset: > > + case XENVER_commandline: > > I'd expect these three to be replaced by default: - all subops should > always be accessible to privileged domains. /me nods. > > Jan >