| Message ID | 576BBABD02000078000F7F14@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote: > >>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote: > > On 06/22/2016 11:23 AM, Jan Beulich wrote: > >>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote: > >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote: > >>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> wrote: > >>>>> I've finally found what was causing long standing issue of not working > >>>>> PCI passthrough for HVM domains with qemu in stubdomain (only - without > >>>>> the other one in dom0). It looks to be this patch: > >>>>> > >>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c428c9f162895cb3473f > >>>>> ab26d23ffbf41a6f293d;hp=dcccaf806a92eabb95929a67c344ac1e9ead6257 > >>>>> > >>>>> It calls xc_domain_getinfo from xc_domain_memory_mapping (to check if > >>>>> the target domain is auto-translated), but xc_domain_getinfo fails with > >>>>> EPERM in stubdomain. > >>>>> > >>>>> What would be the best solution for this? Allowing xc_domain_getinfo > >>>>> from stubdomain in xen/include/xsm/dummy.h? Currently it is uses policy > >>>>> XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have some > >>>>> combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by > >>>>> removing xc_domain_getinfo call in xc_domain_memory_mapping, possibly > >>>>> implementing the logic from that commit solely in libxl? > >>>> > >>>> Once we fixed the quirky behavior of the current implementation > >>>> (allowing information to be returned for other than the requested > >>>> domain), I see no reason why this couldn't become XSM_DM_PRIV. > >>> > >>> Can you explain this more? Is this fix backported to 4.6 and/or 4.4? > >> > >> Which fix? I talked of one to be made. > >> > >>>> But let's ask Daniel explicitly. And in that context I then also wonder > >>>> whether the xsm_getdomaininfo() invocation shouldn't be limited to > >>>> the respective sysctl. > >>> > >>> Actually getdomaininfo is handled in two places in xsm/dummy.h: > >>> - xsm_getdomaininfo (which does nothing when XSM is disabled) > >>> - xsm_domctl (which enforce actual policy) > >>> > >>> Also reading commit message of XSM_XS_PRIV introduction, it may be > >>> useful to be able to just check if given domain is alive, without > >>> getting all the information returned by XEN_DOMCTL_getdomaininfo. I find > >>> this useful also for any other inter-domain communication (for example > >>> libxenvchan connection). > >>> > >>> But for now, XEN_DOMCTL_getdomaininfo should be allowed either when > >>> device-model domain is asking about its target domain, or calling domain > >>> is xenstore domain/privileged domain. Right? > >> > >> Yes, that's what I think too. > >> > >>> How to combine those > >>> types? Change XSM_XS_PRIV to XSM_XS_DM_PRIV (it looks like the only > >>> usage of XSM_XS_PRIV)? > > > > Changing the definition of XSM_XS_PRIV seems like the best solution, since > > this is the only use. I don't think it matters if the constant is renamed > > to XSM_XS_DM_PRIV or not. In fact, since the constant isn't ever used by a > > caller, it could be removed and the actual logic placed in the switch > > statement - that way it's clear this is a special case for getdomaininfo > > instead of attempting to make this generic. > > > > Either method works, and I agree allowing DM to invoke this domctl is both > > useful and not going to introduce problems. The getdomaininfo permission > > will also need to be added to the device_model macro in xen.if. > > What exactly this last sentence means I need to add I'm not sure > about. Apart from that, how about the change below? > > Jan > > domctl: relax getdomaininfo permissions > > Qemu needs access to this for the domain it controls, both due to it > being used by xc_domain_memory_mapping() (which qemu calls) and the > explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll(). > > This at once avoids a for_each_domain() loop when the ID of an > existing domain gets passed in. > > Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > --- > I wonder what good the duplication of the returned domain ID does: I'm > tempted to remove the one in the command-specific structure. Does > anyone have insight into why it was done that way? Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first existing domain with ID >= passed one? Reading various comments in code it looks to be used to domain enumeration. This patch changes this behaviour. > I further wonder why we have XSM_OTHER: The respective conversion into > other XSM_* values in xsm/dummy.h could as well move into the callers, > making intentions more obvious when looking at the actual code. > > --- unstable.orig/xen/common/domctl.c > +++ unstable/xen/common/domctl.c > @@ -442,14 +442,13 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe > switch ( op->cmd ) > { > case XEN_DOMCTL_createdomain: > - case XEN_DOMCTL_getdomaininfo: > case XEN_DOMCTL_test_assign_device: > case XEN_DOMCTL_gdbsx_guestmemio: > d = NULL; > break; > default: > d = rcu_lock_domain_by_id(op->domain); > - if ( d == NULL ) > + if ( !d && op->cmd != XEN_DOMCTL_getdomaininfo ) > return -ESRCH; > } > > @@ -863,14 +862,22 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe > > case XEN_DOMCTL_getdomaininfo: > { > - domid_t dom = op->domain; > - > - rcu_read_lock(&domlist_read_lock); > + domid_t dom = DOMID_INVALID; > > - for_each_domain ( d ) > - if ( d->domain_id >= dom ) > + if ( !d ) > + { > + ret = -EINVAL; > + if ( op->domain >= DOMID_FIRST_RESERVED ) > break; > > + rcu_read_lock(&domlist_read_lock); > + > + dom = op->domain; > + for_each_domain ( d ) > + if ( d->domain_id >= dom ) > + break; > + } > + > ret = -ESRCH; > if ( d == NULL ) > goto getdomaininfo_out; > @@ -885,6 +892,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe > copyback = 1; > > getdomaininfo_out: > + if ( dom == DOMID_INVALID ) > + break; > + > rcu_read_unlock(&domlist_read_lock); > d = NULL; > break; > --- unstable.orig/xen/include/xsm/dummy.h > +++ unstable/xen/include/xsm/dummy.h > @@ -61,7 +61,12 @@ static always_inline int xsm_default_act > return 0; > case XSM_TARGET: > if ( src == target ) > + { > return 0; > + case XSM_XS_PRIV: > + if ( src->is_xenstore ) > + return 0; > + } > /* fall through */ > case XSM_DM_PRIV: > if ( target && src->target == target ) > @@ -71,10 +76,6 @@ static always_inline int xsm_default_act > if ( src->is_privileged ) > return 0; > return -EPERM; > - case XSM_XS_PRIV: > - if ( src->is_xenstore || src->is_privileged ) > - return 0; > - return -EPERM; > default: > LINKER_BUG_ON(1); > return -EPERM; > >
>>> On 23.06.16 at 10:57, <marmarek@invisiblethingslab.com> wrote: > On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote: >> >>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote: >> > On 06/22/2016 11:23 AM, Jan Beulich wrote: >> >>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote: >> >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote: >> >>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> wrote: >> >>>>> I've finally found what was causing long standing issue of not working >> >>>>> PCI passthrough for HVM domains with qemu in stubdomain (only - without >> >>>>> the other one in dom0). It looks to be this patch: >> >>>>> >> >>> > http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c428c9f162895cb3473f >> >>>>> ab26d23ffbf41a6f293d;hp=dcccaf806a92eabb95929a67c344ac1e9ead6257 >> >>>>> >> >>>>> It calls xc_domain_getinfo from xc_domain_memory_mapping (to check if >> >>>>> the target domain is auto-translated), but xc_domain_getinfo fails with >> >>>>> EPERM in stubdomain. >> >>>>> >> >>>>> What would be the best solution for this? Allowing xc_domain_getinfo >> >>>>> from stubdomain in xen/include/xsm/dummy.h? Currently it is uses policy >> >>>>> XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have some >> >>>>> combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by >> >>>>> removing xc_domain_getinfo call in xc_domain_memory_mapping, possibly >> >>>>> implementing the logic from that commit solely in libxl? >> >>>> >> >>>> Once we fixed the quirky behavior of the current implementation >> >>>> (allowing information to be returned for other than the requested >> >>>> domain), I see no reason why this couldn't become XSM_DM_PRIV. >> >>> >> >>> Can you explain this more? Is this fix backported to 4.6 and/or 4.4? >> >> >> >> Which fix? I talked of one to be made. >> >> >> >>>> But let's ask Daniel explicitly. And in that context I then also wonder >> >>>> whether the xsm_getdomaininfo() invocation shouldn't be limited to >> >>>> the respective sysctl. >> >>> >> >>> Actually getdomaininfo is handled in two places in xsm/dummy.h: >> >>> - xsm_getdomaininfo (which does nothing when XSM is disabled) >> >>> - xsm_domctl (which enforce actual policy) >> >>> >> >>> Also reading commit message of XSM_XS_PRIV introduction, it may be >> >>> useful to be able to just check if given domain is alive, without >> >>> getting all the information returned by XEN_DOMCTL_getdomaininfo. I find >> >>> this useful also for any other inter-domain communication (for example >> >>> libxenvchan connection). >> >>> >> >>> But for now, XEN_DOMCTL_getdomaininfo should be allowed either when >> >>> device-model domain is asking about its target domain, or calling domain >> >>> is xenstore domain/privileged domain. Right? >> >> >> >> Yes, that's what I think too. >> >> >> >>> How to combine those >> >>> types? Change XSM_XS_PRIV to XSM_XS_DM_PRIV (it looks like the only >> >>> usage of XSM_XS_PRIV)? >> > >> > Changing the definition of XSM_XS_PRIV seems like the best solution, since >> > this is the only use. I don't think it matters if the constant is renamed >> > to XSM_XS_DM_PRIV or not. In fact, since the constant isn't ever used by a >> > caller, it could be removed and the actual logic placed in the switch >> > statement - that way it's clear this is a special case for getdomaininfo >> > instead of attempting to make this generic. >> > >> > Either method works, and I agree allowing DM to invoke this domctl is both >> > useful and not going to introduce problems. The getdomaininfo permission >> > will also need to be added to the device_model macro in xen.if. >> >> What exactly this last sentence means I need to add I'm not sure >> about. Apart from that, how about the change below? >> >> Jan >> >> domctl: relax getdomaininfo permissions >> >> Qemu needs access to this for the domain it controls, both due to it >> being used by xc_domain_memory_mapping() (which qemu calls) and the >> explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll(). >> >> This at once avoids a for_each_domain() loop when the ID of an >> existing domain gets passed in. >> >> Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> >> --- >> I wonder what good the duplication of the returned domain ID does: I'm >> tempted to remove the one in the command-specific structure. Does >> anyone have insight into why it was done that way? > > Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first > existing domain with ID >= passed one? Reading various comments in code > it looks to be used to domain enumeration. This patch changes this > behaviour. No, it doesn't. It adjusts the behavior only for the DM case (which isn't supposed to get information on other than the target domain, i.e. in this one specific case the very domain ID needs to be passed in). Also, how is this comment of yours related to the remark above? Jan
On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote: > >>> On 23.06.16 at 10:57, <marmarek@invisiblethingslab.com> wrote: > > On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote: > >> >>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote: > >> > On 06/22/2016 11:23 AM, Jan Beulich wrote: > >> >>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote: > >> >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote: > >> >>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> wrote: > >> >>>>> I've finally found what was causing long standing issue of not working > >> >>>>> PCI passthrough for HVM domains with qemu in stubdomain (only - without > >> >>>>> the other one in dom0). It looks to be this patch: > >> >>>>> > >> >>> > > http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c428c9f162895cb3473f > >> >>>>> ab26d23ffbf41a6f293d;hp=dcccaf806a92eabb95929a67c344ac1e9ead6257 > >> >>>>> > >> >>>>> It calls xc_domain_getinfo from xc_domain_memory_mapping (to check if > >> >>>>> the target domain is auto-translated), but xc_domain_getinfo fails with > >> >>>>> EPERM in stubdomain. > >> >>>>> > >> >>>>> What would be the best solution for this? Allowing xc_domain_getinfo > >> >>>>> from stubdomain in xen/include/xsm/dummy.h? Currently it is uses policy > >> >>>>> XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have some > >> >>>>> combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by > >> >>>>> removing xc_domain_getinfo call in xc_domain_memory_mapping, possibly > >> >>>>> implementing the logic from that commit solely in libxl? > >> >>>> > >> >>>> Once we fixed the quirky behavior of the current implementation > >> >>>> (allowing information to be returned for other than the requested > >> >>>> domain), I see no reason why this couldn't become XSM_DM_PRIV. > >> >>> > >> >>> Can you explain this more? Is this fix backported to 4.6 and/or 4.4? > >> >> > >> >> Which fix? I talked of one to be made. > >> >> > >> >>>> But let's ask Daniel explicitly. And in that context I then also wonder > >> >>>> whether the xsm_getdomaininfo() invocation shouldn't be limited to > >> >>>> the respective sysctl. > >> >>> > >> >>> Actually getdomaininfo is handled in two places in xsm/dummy.h: > >> >>> - xsm_getdomaininfo (which does nothing when XSM is disabled) > >> >>> - xsm_domctl (which enforce actual policy) > >> >>> > >> >>> Also reading commit message of XSM_XS_PRIV introduction, it may be > >> >>> useful to be able to just check if given domain is alive, without > >> >>> getting all the information returned by XEN_DOMCTL_getdomaininfo. I find > >> >>> this useful also for any other inter-domain communication (for example > >> >>> libxenvchan connection). > >> >>> > >> >>> But for now, XEN_DOMCTL_getdomaininfo should be allowed either when > >> >>> device-model domain is asking about its target domain, or calling domain > >> >>> is xenstore domain/privileged domain. Right? > >> >> > >> >> Yes, that's what I think too. > >> >> > >> >>> How to combine those > >> >>> types? Change XSM_XS_PRIV to XSM_XS_DM_PRIV (it looks like the only > >> >>> usage of XSM_XS_PRIV)? > >> > > >> > Changing the definition of XSM_XS_PRIV seems like the best solution, since > >> > this is the only use. I don't think it matters if the constant is renamed > >> > to XSM_XS_DM_PRIV or not. In fact, since the constant isn't ever used by a > >> > caller, it could be removed and the actual logic placed in the switch > >> > statement - that way it's clear this is a special case for getdomaininfo > >> > instead of attempting to make this generic. > >> > > >> > Either method works, and I agree allowing DM to invoke this domctl is both > >> > useful and not going to introduce problems. The getdomaininfo permission > >> > will also need to be added to the device_model macro in xen.if. > >> > >> What exactly this last sentence means I need to add I'm not sure > >> about. Apart from that, how about the change below? > >> > >> Jan > >> > >> domctl: relax getdomaininfo permissions > >> > >> Qemu needs access to this for the domain it controls, both due to it > >> being used by xc_domain_memory_mapping() (which qemu calls) and the > >> explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll(). > >> > >> This at once avoids a for_each_domain() loop when the ID of an > >> existing domain gets passed in. > >> > >> Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > >> --- > >> I wonder what good the duplication of the returned domain ID does: I'm > >> tempted to remove the one in the command-specific structure. Does > >> anyone have insight into why it was done that way? > > > > Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first > > existing domain with ID >= passed one? Reading various comments in code > > it looks to be used to domain enumeration. This patch changes this > > behaviour. > > No, it doesn't. It adjusts the behavior only for the DM case (which > isn't supposed to get information on other than the target domain, > i.e. in this one specific case the very domain ID needs to be passed > in). int xc_domain_getinfo(xc_interface *xch, uint32_t first_domid, unsigned int max_doms, xc_dominfo_t *info) { unsigned int nr_doms; uint32_t next_domid = first_domid; DECLARE_DOMCTL; int rc = 0; memset(info, 0, max_doms*sizeof(xc_dominfo_t)); for ( nr_doms = 0; nr_doms < max_doms; nr_doms++ ) { domctl.cmd = XEN_DOMCTL_getdomaininfo; domctl.domain = (domid_t)next_domid; if ( (rc = do_domctl(xch, &domctl)) < 0 ) break; info->domid = (uint16_t)domctl.domain; (...) next_domid = (uint16_t)domctl.domain + 1; Looks like heavily dependent on XEN_DOMCTL_getdomaininfo returning next valid domain. > Also, how is this comment of yours related to the remark above? Because this is why domid is needed in returned structure - to know about which domain you've got the info.
On Thu, Jun 23, 2016 at 11:18:24AM +0200, Marek Marczykowski-Górecki wrote: > On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote: > > >>> On 23.06.16 at 10:57, <marmarek@invisiblethingslab.com> wrote: > > > On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote: > > >> >>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote: > > >> > On 06/22/2016 11:23 AM, Jan Beulich wrote: > > >> >>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote: > > >> >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote: > > >> >>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> wrote: > > >> >>>>> I've finally found what was causing long standing issue of not working > > >> >>>>> PCI passthrough for HVM domains with qemu in stubdomain (only - without > > >> >>>>> the other one in dom0). It looks to be this patch: > > >> >>>>> > > >> >>> > > > http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c428c9f162895cb3473f > > >> >>>>> ab26d23ffbf41a6f293d;hp=dcccaf806a92eabb95929a67c344ac1e9ead6257 > > >> >>>>> > > >> >>>>> It calls xc_domain_getinfo from xc_domain_memory_mapping (to check if > > >> >>>>> the target domain is auto-translated), but xc_domain_getinfo fails with > > >> >>>>> EPERM in stubdomain. > > >> >>>>> > > >> >>>>> What would be the best solution for this? Allowing xc_domain_getinfo > > >> >>>>> from stubdomain in xen/include/xsm/dummy.h? Currently it is uses policy > > >> >>>>> XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have some > > >> >>>>> combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by > > >> >>>>> removing xc_domain_getinfo call in xc_domain_memory_mapping, possibly > > >> >>>>> implementing the logic from that commit solely in libxl? > > >> >>>> > > >> >>>> Once we fixed the quirky behavior of the current implementation > > >> >>>> (allowing information to be returned for other than the requested > > >> >>>> domain), I see no reason why this couldn't become XSM_DM_PRIV. > > >> >>> > > >> >>> Can you explain this more? Is this fix backported to 4.6 and/or 4.4? > > >> >> > > >> >> Which fix? I talked of one to be made. > > >> >> > > >> >>>> But let's ask Daniel explicitly. And in that context I then also wonder > > >> >>>> whether the xsm_getdomaininfo() invocation shouldn't be limited to > > >> >>>> the respective sysctl. > > >> >>> > > >> >>> Actually getdomaininfo is handled in two places in xsm/dummy.h: > > >> >>> - xsm_getdomaininfo (which does nothing when XSM is disabled) > > >> >>> - xsm_domctl (which enforce actual policy) > > >> >>> > > >> >>> Also reading commit message of XSM_XS_PRIV introduction, it may be > > >> >>> useful to be able to just check if given domain is alive, without > > >> >>> getting all the information returned by XEN_DOMCTL_getdomaininfo. I find > > >> >>> this useful also for any other inter-domain communication (for example > > >> >>> libxenvchan connection). > > >> >>> > > >> >>> But for now, XEN_DOMCTL_getdomaininfo should be allowed either when > > >> >>> device-model domain is asking about its target domain, or calling domain > > >> >>> is xenstore domain/privileged domain. Right? > > >> >> > > >> >> Yes, that's what I think too. > > >> >> > > >> >>> How to combine those > > >> >>> types? Change XSM_XS_PRIV to XSM_XS_DM_PRIV (it looks like the only > > >> >>> usage of XSM_XS_PRIV)? > > >> > > > >> > Changing the definition of XSM_XS_PRIV seems like the best solution, since > > >> > this is the only use. I don't think it matters if the constant is renamed > > >> > to XSM_XS_DM_PRIV or not. In fact, since the constant isn't ever used by a > > >> > caller, it could be removed and the actual logic placed in the switch > > >> > statement - that way it's clear this is a special case for getdomaininfo > > >> > instead of attempting to make this generic. > > >> > > > >> > Either method works, and I agree allowing DM to invoke this domctl is both > > >> > useful and not going to introduce problems. The getdomaininfo permission > > >> > will also need to be added to the device_model macro in xen.if. > > >> > > >> What exactly this last sentence means I need to add I'm not sure > > >> about. Apart from that, how about the change below? > > >> > > >> Jan > > >> > > >> domctl: relax getdomaininfo permissions > > >> > > >> Qemu needs access to this for the domain it controls, both due to it > > >> being used by xc_domain_memory_mapping() (which qemu calls) and the > > >> explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll(). > > >> > > >> This at once avoids a for_each_domain() loop when the ID of an > > >> existing domain gets passed in. > > >> > > >> Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > >> --- > > >> I wonder what good the duplication of the returned domain ID does: I'm > > >> tempted to remove the one in the command-specific structure. Does > > >> anyone have insight into why it was done that way? > > > > > > Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first > > > existing domain with ID >= passed one? Reading various comments in code > > > it looks to be used to domain enumeration. This patch changes this > > > behaviour. > > > > No, it doesn't. It adjusts the behavior only for the DM case (which > > isn't supposed to get information on other than the target domain, > > i.e. in this one specific case the very domain ID needs to be passed > > in). > > int xc_domain_getinfo(xc_interface *xch, > uint32_t first_domid, > unsigned int max_doms, > xc_dominfo_t *info) > { > unsigned int nr_doms; > uint32_t next_domid = first_domid; > DECLARE_DOMCTL; > int rc = 0; > > memset(info, 0, max_doms*sizeof(xc_dominfo_t)); > > for ( nr_doms = 0; nr_doms < max_doms; nr_doms++ ) > { > domctl.cmd = XEN_DOMCTL_getdomaininfo; > domctl.domain = (domid_t)next_domid; > if ( (rc = do_domctl(xch, &domctl)) < 0 ) > break; > info->domid = (uint16_t)domctl.domain; > (...) > next_domid = (uint16_t)domctl.domain + 1; > > > Looks like heavily dependent on XEN_DOMCTL_getdomaininfo returning next valid > domain. Hmm, looks like I've misread you patch. Reading again... But now I see rcu_read_lock(&domlist_read_lock) is gets called only when looping over domains, but rcu_read_unlock is called in any case. Is it correct? > > Also, how is this comment of yours related to the remark above? > > Because this is why domid is needed in returned structure - to know about which > domain you've got the info. >
On 23/06/16 09:32, Jan Beulich wrote: >>>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote: >> On 06/22/2016 11:23 AM, Jan Beulich wrote: >>>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote: >>>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote: >>>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> wrote: >>>>>> I've finally found what was causing long standing issue of not working >>>>>> PCI passthrough for HVM domains with qemu in stubdomain (only - without >>>>>> the other one in dom0). It looks to be this patch: >>>>>> >>>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c428c9f162895cb3473f >>>>>> ab26d23ffbf41a6f293d;hp=dcccaf806a92eabb95929a67c344ac1e9ead6257 >>>>>> >>>>>> It calls xc_domain_getinfo from xc_domain_memory_mapping (to check if >>>>>> the target domain is auto-translated), but xc_domain_getinfo fails with >>>>>> EPERM in stubdomain. >>>>>> >>>>>> What would be the best solution for this? Allowing xc_domain_getinfo >>>>>> from stubdomain in xen/include/xsm/dummy.h? Currently it is uses policy >>>>>> XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have some >>>>>> combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by >>>>>> removing xc_domain_getinfo call in xc_domain_memory_mapping, possibly >>>>>> implementing the logic from that commit solely in libxl? >>>>> Once we fixed the quirky behavior of the current implementation >>>>> (allowing information to be returned for other than the requested >>>>> domain), I see no reason why this couldn't become XSM_DM_PRIV. >>>> Can you explain this more? Is this fix backported to 4.6 and/or 4.4? >>> Which fix? I talked of one to be made. >>> >>>>> But let's ask Daniel explicitly. And in that context I then also wonder >>>>> whether the xsm_getdomaininfo() invocation shouldn't be limited to >>>>> the respective sysctl. >>>> Actually getdomaininfo is handled in two places in xsm/dummy.h: >>>> - xsm_getdomaininfo (which does nothing when XSM is disabled) >>>> - xsm_domctl (which enforce actual policy) >>>> >>>> Also reading commit message of XSM_XS_PRIV introduction, it may be >>>> useful to be able to just check if given domain is alive, without >>>> getting all the information returned by XEN_DOMCTL_getdomaininfo. I find >>>> this useful also for any other inter-domain communication (for example >>>> libxenvchan connection). >>>> >>>> But for now, XEN_DOMCTL_getdomaininfo should be allowed either when >>>> device-model domain is asking about its target domain, or calling domain >>>> is xenstore domain/privileged domain. Right? >>> Yes, that's what I think too. >>> >>>> How to combine those >>>> types? Change XSM_XS_PRIV to XSM_XS_DM_PRIV (it looks like the only >>>> usage of XSM_XS_PRIV)? >> Changing the definition of XSM_XS_PRIV seems like the best solution, since >> this is the only use. I don't think it matters if the constant is renamed >> to XSM_XS_DM_PRIV or not. In fact, since the constant isn't ever used by a >> caller, it could be removed and the actual logic placed in the switch >> statement - that way it's clear this is a special case for getdomaininfo >> instead of attempting to make this generic. >> >> Either method works, and I agree allowing DM to invoke this domctl is both >> useful and not going to introduce problems. The getdomaininfo permission >> will also need to be added to the device_model macro in xen.if. > What exactly this last sentence means I need to add I'm not sure > about. Apart from that, how about the change below? > > Jan > > domctl: relax getdomaininfo permissions > > Qemu needs access to this for the domain it controls, both due to it > being used by xc_domain_memory_mapping() (which qemu calls) and the > explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll(). > > This at once avoids a for_each_domain() loop when the ID of an > existing domain gets passed in. > > Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > --- > I wonder what good the duplication of the returned domain ID does: I'm > tempted to remove the one in the command-specific structure. Does > anyone have insight into why it was done that way? This hypercall, and its sibling XEN_SYSCTL_getdomaininfolist have crazy semantics, which go out of their way to make it easy to get wrong. It is important that you always check the returned domid, as it may not be the domain you asked for. In particular, if a domain you are looking after dies, the adjacent domain's information will be returnned. Also, noone has yet addressed the issue that, strictly speaking, xen_domctl_getdomaininfo_t is versioned by both the DOMCTL and the SYSCTL interface version. This in particular makes things interesting for valgrind support. ~Andrew
>>> On 23.06.16 at 11:18, <marmarek@invisiblethingslab.com> wrote: > On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote: >> >>> On 23.06.16 at 10:57, <marmarek@invisiblethingslab.com> wrote: >> > On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote: >> >> >>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote: >> >> > On 06/22/2016 11:23 AM, Jan Beulich wrote: >> >> >>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote: >> >> >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote: >> >> >>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> wrote: >> >> >>>>> I've finally found what was causing long standing issue of not working >> >> >>>>> PCI passthrough for HVM domains with qemu in stubdomain (only - without >> >> >>>>> the other one in dom0). It looks to be this patch: >> >> >>>>> >> >> >>> >> > > http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c428c9f162895cb3473f >> >> >>>>> ab26d23ffbf41a6f293d;hp=dcccaf806a92eabb95929a67c344ac1e9ead6257 >> >> >>>>> >> >> >>>>> It calls xc_domain_getinfo from xc_domain_memory_mapping (to check if >> >> >>>>> the target domain is auto-translated), but xc_domain_getinfo fails with >> >> >>>>> EPERM in stubdomain. >> >> >>>>> >> >> >>>>> What would be the best solution for this? Allowing xc_domain_getinfo >> >> >>>>> from stubdomain in xen/include/xsm/dummy.h? Currently it is uses policy >> >> >>>>> XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have some >> >> >>>>> combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by >> >> >>>>> removing xc_domain_getinfo call in xc_domain_memory_mapping, possibly >> >> >>>>> implementing the logic from that commit solely in libxl? >> >> >>>> >> >> >>>> Once we fixed the quirky behavior of the current implementation >> >> >>>> (allowing information to be returned for other than the requested >> >> >>>> domain), I see no reason why this couldn't become XSM_DM_PRIV. >> >> >>> >> >> >>> Can you explain this more? Is this fix backported to 4.6 and/or 4.4? >> >> >> >> >> >> Which fix? I talked of one to be made. >> >> >> >> >> >>>> But let's ask Daniel explicitly. And in that context I then also wonder >> >> >>>> whether the xsm_getdomaininfo() invocation shouldn't be limited to >> >> >>>> the respective sysctl. >> >> >>> >> >> >>> Actually getdomaininfo is handled in two places in xsm/dummy.h: >> >> >>> - xsm_getdomaininfo (which does nothing when XSM is disabled) >> >> >>> - xsm_domctl (which enforce actual policy) >> >> >>> >> >> >>> Also reading commit message of XSM_XS_PRIV introduction, it may be >> >> >>> useful to be able to just check if given domain is alive, without >> >> >>> getting all the information returned by XEN_DOMCTL_getdomaininfo. I find >> >> >>> this useful also for any other inter-domain communication (for example >> >> >>> libxenvchan connection). >> >> >>> >> >> >>> But for now, XEN_DOMCTL_getdomaininfo should be allowed either when >> >> >>> device-model domain is asking about its target domain, or calling domain >> >> >>> is xenstore domain/privileged domain. Right? >> >> >> >> >> >> Yes, that's what I think too. >> >> >> >> >> >>> How to combine those >> >> >>> types? Change XSM_XS_PRIV to XSM_XS_DM_PRIV (it looks like the only >> >> >>> usage of XSM_XS_PRIV)? >> >> > >> >> > Changing the definition of XSM_XS_PRIV seems like the best solution, since >> >> > this is the only use. I don't think it matters if the constant is renamed >> >> > to XSM_XS_DM_PRIV or not. In fact, since the constant isn't ever used by > a >> >> > caller, it could be removed and the actual logic placed in the switch >> >> > statement - that way it's clear this is a special case for getdomaininfo >> >> > instead of attempting to make this generic. >> >> > >> >> > Either method works, and I agree allowing DM to invoke this domctl is both >> >> > useful and not going to introduce problems. The getdomaininfo permission >> >> > will also need to be added to the device_model macro in xen.if. >> >> >> >> What exactly this last sentence means I need to add I'm not sure >> >> about. Apart from that, how about the change below? >> >> >> >> Jan >> >> >> >> domctl: relax getdomaininfo permissions >> >> >> >> Qemu needs access to this for the domain it controls, both due to it >> >> being used by xc_domain_memory_mapping() (which qemu calls) and the >> >> explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll(). >> >> >> >> This at once avoids a for_each_domain() loop when the ID of an >> >> existing domain gets passed in. >> >> >> >> Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> >> >> --- >> >> I wonder what good the duplication of the returned domain ID does: I'm >> >> tempted to remove the one in the command-specific structure. Does >> >> anyone have insight into why it was done that way? >> > >> > Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first >> > existing domain with ID >= passed one? Reading various comments in code >> > it looks to be used to domain enumeration. This patch changes this >> > behaviour. >> >> No, it doesn't. It adjusts the behavior only for the DM case (which >> isn't supposed to get information on other than the target domain, >> i.e. in this one specific case the very domain ID needs to be passed >> in). > > int xc_domain_getinfo(xc_interface *xch, > uint32_t first_domid, > unsigned int max_doms, > xc_dominfo_t *info) > { > unsigned int nr_doms; > uint32_t next_domid = first_domid; > DECLARE_DOMCTL; > int rc = 0; > > memset(info, 0, max_doms*sizeof(xc_dominfo_t)); > > for ( nr_doms = 0; nr_doms < max_doms; nr_doms++ ) > { > domctl.cmd = XEN_DOMCTL_getdomaininfo; > domctl.domain = (domid_t)next_domid; > if ( (rc = do_domctl(xch, &domctl)) < 0 ) > break; > info->domid = (uint16_t)domctl.domain; > (...) > next_domid = (uint16_t)domctl.domain + 1; > > > Looks like heavily dependent on XEN_DOMCTL_getdomaininfo returning next > valid > domain. Please consider the case of max_doms == 1, which is the only one of interest here (and the only one getting tweaked). >> Also, how is this comment of yours related to the remark above? > > Because this is why domid is needed in returned structure - to know about > which > domain you've got the info. I'm sorry, but no - this does not explain the duplication. Just look at the code you quoted: It uses domctl.domain, but completely ignores domctl.u.getdomaininfo.domain. Jan
>>> On 23.06.16 at 11:23, <marmarek@invisiblethingslab.com> wrote: > On Thu, Jun 23, 2016 at 11:18:24AM +0200, Marek Marczykowski-Górecki wrote: >> On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote: >> > >>> On 23.06.16 at 10:57, <marmarek@invisiblethingslab.com> wrote: >> > > On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote: >> > >> I wonder what good the duplication of the returned domain ID does: I'm >> > >> tempted to remove the one in the command-specific structure. Does >> > >> anyone have insight into why it was done that way? >> > > >> > > Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first >> > > existing domain with ID >= passed one? Reading various comments in code >> > > it looks to be used to domain enumeration. This patch changes this >> > > behaviour. >> > >> > No, it doesn't. It adjusts the behavior only for the DM case (which >> > isn't supposed to get information on other than the target domain, >> > i.e. in this one specific case the very domain ID needs to be passed >> > in). >> >> int xc_domain_getinfo(xc_interface *xch, >> uint32_t first_domid, >> unsigned int max_doms, >> xc_dominfo_t *info) >> { >> unsigned int nr_doms; >> uint32_t next_domid = first_domid; >> DECLARE_DOMCTL; >> int rc = 0; >> >> memset(info, 0, max_doms*sizeof(xc_dominfo_t)); >> >> for ( nr_doms = 0; nr_doms < max_doms; nr_doms++ ) >> { >> domctl.cmd = XEN_DOMCTL_getdomaininfo; >> domctl.domain = (domid_t)next_domid; >> if ( (rc = do_domctl(xch, &domctl)) < 0 ) >> break; >> info->domid = (uint16_t)domctl.domain; >> (...) >> next_domid = (uint16_t)domctl.domain + 1; >> >> >> Looks like heavily dependent on XEN_DOMCTL_getdomaininfo returning next > valid >> domain. > > Hmm, looks like I've misread you patch. Reading again... > > But now I see rcu_read_lock(&domlist_read_lock) is gets called only when > looping over domains, but rcu_read_unlock is called in any case. Is it > correct? How that? There is this third hunk: @@ -885,6 +892,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe copyback = 1; getdomaininfo_out: + if ( dom == DOMID_INVALID ) + break; + rcu_read_unlock(&domlist_read_lock); d = NULL; break; Jan
>>> On 23.06.16 at 11:44, <andrew.cooper3@citrix.com> wrote: > On 23/06/16 09:32, Jan Beulich wrote: >> I wonder what good the duplication of the returned domain ID does: I'm >> tempted to remove the one in the command-specific structure. Does >> anyone have insight into why it was done that way? > > This hypercall, and its sibling XEN_SYSCTL_getdomaininfolist have crazy > semantics, which go out of their way to make it easy to get wrong. > > It is important that you always check the returned domid, as it may not > be the domain you asked for. In particular, if a domain you are looking > after dies, the adjacent domain's information will be returnned. Same question as to Marek: How is this related to my remark? > Also, noone has yet addressed the issue that, strictly speaking, > xen_domctl_getdomaininfo_t is versioned by both the DOMCTL and the > SYSCTL interface version. This in particular makes things interesting > for valgrind support. True, and afaict also the case for XEN_SCHEDULER_*. No idea how to cleanly address this other than by folding the two versions. Jan
On 23/06/16 10:50, Jan Beulich wrote: >>>> On 23.06.16 at 11:44, <andrew.cooper3@citrix.com> wrote: >> On 23/06/16 09:32, Jan Beulich wrote: >>> I wonder what good the duplication of the returned domain ID does: I'm >>> tempted to remove the one in the command-specific structure. Does >>> anyone have insight into why it was done that way? >> This hypercall, and its sibling XEN_SYSCTL_getdomaininfolist have crazy >> semantics, which go out of their way to make it easy to get wrong. >> >> It is important that you always check the returned domid, as it may not >> be the domain you asked for. In particular, if a domain you are looking >> after dies, the adjacent domain's information will be returnned. > Same question as to Marek: How is this related to my remark? Oh right. I misread. Altering the domctl domid value is pointless, as libxc abstracts the call behind do_domctl() anyway. ~Andrew
--- unstable.orig/xen/common/domctl.c +++ unstable/xen/common/domctl.c @@ -442,14 +442,13 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe switch ( op->cmd ) { case XEN_DOMCTL_createdomain: - case XEN_DOMCTL_getdomaininfo: case XEN_DOMCTL_test_assign_device: case XEN_DOMCTL_gdbsx_guestmemio: d = NULL; break; default: d = rcu_lock_domain_by_id(op->domain); - if ( d == NULL ) + if ( !d && op->cmd != XEN_DOMCTL_getdomaininfo ) return -ESRCH; } @@ -863,14 +862,22 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe case XEN_DOMCTL_getdomaininfo: { - domid_t dom = op->domain; - - rcu_read_lock(&domlist_read_lock); + domid_t dom = DOMID_INVALID; - for_each_domain ( d ) - if ( d->domain_id >= dom ) + if ( !d ) + { + ret = -EINVAL; + if ( op->domain >= DOMID_FIRST_RESERVED ) break; + rcu_read_lock(&domlist_read_lock); + + dom = op->domain; + for_each_domain ( d ) + if ( d->domain_id >= dom ) + break; + } + ret = -ESRCH; if ( d == NULL ) goto getdomaininfo_out; @@ -885,6 +892,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe copyback = 1; getdomaininfo_out: + if ( dom == DOMID_INVALID ) + break; + rcu_read_unlock(&domlist_read_lock); d = NULL; break; --- unstable.orig/xen/include/xsm/dummy.h +++ unstable/xen/include/xsm/dummy.h @@ -61,7 +61,12 @@ static always_inline int xsm_default_act return 0; case XSM_TARGET: if ( src == target ) + { return 0; + case XSM_XS_PRIV: + if ( src->is_xenstore ) + return 0; + } /* fall through */ case XSM_DM_PRIV: if ( target && src->target == target ) @@ -71,10 +76,6 @@ static always_inline int xsm_default_act if ( src->is_privileged ) return 0; return -EPERM; - case XSM_XS_PRIV: - if ( src->is_xenstore || src->is_privileged ) - return 0; - return -EPERM; default: LINKER_BUG_ON(1); return -EPERM;