| Message ID | 20160623152247.GK1593@mail-itl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/23/2016 11:22 AM, Marek Marczykowski-Górecki wrote: > On Thu, Jun 23, 2016 at 11:00:42AM -0400, Daniel De Graaf wrote: >> On 06/23/2016 09:25 AM, Marek Marczykowski-Górecki wrote: >> [...] >>> Ok, after drawing a flowchart of the control in this function after your >>> change, on a piece of paper, this case looks fine. But depending on how >>> the domain was found (explicit loop or rcu_lock_domain_by_id), different >>> locks are held, which makes it harder to follow what is going on. >>> >>> Crazy idea: how about making the code easy/easier to read instead of >>> obfuscating it even more? XEN_DOMCTL_getdomaininfo semantic is >>> convolved enough. How about this version (2 patches): >> [...] >>> xen: allow XEN_DOMCTL_getdomaininfo for device model >>> >>> Allow device model domain to get info about its target domain. >>> It is used during PCI passthrough setup (xc_domain_memory_mapping >>> checks for guest being auto-translated). While it happens in stubdomain, >>> it failed, breaking PCI passthrough in such setup. >>> >>> While it is possible to workaround this at toolstack side, it seems >>> logical to allow device model to get information about its target >>> domain. >>> >>> The problem was exposed by c428c9f "tools/libxl: handle the iomem >>> parameter with the memory_mapping hcall". >>> >>> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> >>> --- >>> xen/include/xsm/dummy.h | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h >>> index 406cd18..70a1633 100644 >>> --- a/xen/include/xsm/dummy.h >>> +++ b/xen/include/xsm/dummy.h >>> @@ -128,7 +128,10 @@ static XSM_INLINE int xsm_domctl(XSM_DEFAULT_ARG struct domain *d, int cmd) >>> case XEN_DOMCTL_unbind_pt_irq: >>> return xsm_default_action(XSM_DM_PRIV, current->domain, d); >>> case XEN_DOMCTL_getdomaininfo: >>> - return xsm_default_action(XSM_XS_PRIV, current->domain, d); >>> + if (current->domain->target) >>> + return xsm_default_action(XSM_DM_PRIV, current->domain, d); >>> + else >>> + return xsm_default_action(XSM_XS_PRIV, current->domain, d); >>> default: >>> return xsm_default_action(XSM_PRIV, current->domain, d); >>> } >> >> I would prefer testing for the xenstore flag instead of testing for the >> target field. It ends up being the same thing in reality, since nobody >> sane would make the xenstore also a device model (and not also dom0). >> >> case XEN_DOMCTL_getdomaininfo: >> if ( src->is_xenstore ) >> return 0; >> return xsm_default_action(XSM_DM_PRIV, current->domain, d); >> >> This makes it clear that xenstore is the special case, and removes the >> need for the one-off XSM_XS_PRIV constant. > > This was my initial idea, but I don't really understand the comment > about link-time verification if the behaviour is the same for xsm not > compiled vs disabled. But if skipping xsm_default_action here doesn't > break this magic, I'm for it. That magic is just for the parameter to the XSM hook: in this case, it's the XSM_OTHER in xsm_domctl that is being verified. There is no magic in xsm_default_action. > Updated patch (with removal of XSM_XS_PRIV): > > xen: allow XEN_DOMCTL_getdomaininfo for device model domains > > Allow device model domain to get info about its target domain. > It is used during PCI passthrough setup (xc_domain_memory_mapping > checks for guest being auto-translated). While it happens in stubdomain, > it failed, breaking PCI passthrough in such setup. > > While it is possible to workaround this at toolstack side, it seems > logical to allow device model to get information about its target > domain. > > Also, since this was the only usage of XSM_XS_PRIV, which now gets > handled inline, drop it. > > The problem was exposed by c428c9f "tools/libxl: handle the iomem > parameter with the memory_mapping hcall". > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> This would be a good patch to include the corresponding change to the XSM policy (tweaked from Jan's email): > --- a/tools/flask/policy/modules/xen/xen.if > +++ b/tools/flask/policy/modules/xen/xen.if > @@ -148,7 +148,7 @@ define(`device_model', ` > create_channel($2, $1, $2_channel) > allow $1 $2_channel:event create; > > - allow $1 $2_target:domain shutdown; > + allow $1 $2_target:domain { getdomaininfo shutdown }; > allow $1 $2_target:mmu { map_read map_write adjust physmap target_hack }; > allow $1 $2_target:hvm { getparam setparam trackdirtyvram hvmctl irqlevel pciroute pcilevel cacheattr send_irq }; > ') With that included, or elsewhere in the series: Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> > --- > xen/include/xsm/dummy.h | 8 +++----- > xen/include/xsm/xsm.h | 1 - > 2 files changed, 3 insertions(+), 6 deletions(-) > > diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h > index 406cd18..2768861 100644 > --- a/xen/include/xsm/dummy.h > +++ b/xen/include/xsm/dummy.h > @@ -71,10 +71,6 @@ static always_inline int xsm_default_action( > 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; > @@ -128,7 +124,9 @@ static XSM_INLINE int xsm_domctl(XSM_DEFAULT_ARG struct domain *d, int cmd) > case XEN_DOMCTL_unbind_pt_irq: > return xsm_default_action(XSM_DM_PRIV, current->domain, d); > case XEN_DOMCTL_getdomaininfo: > - return xsm_default_action(XSM_XS_PRIV, current->domain, d); > + if ( current->domain->is_xenstore ) > + return 0; > + return xsm_default_action(XSM_DM_PRIV, current->domain, d); > default: > return xsm_default_action(XSM_PRIV, current->domain, d); > } > diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h > index 0d525ec..09672e7 100644 > --- a/xen/include/xsm/xsm.h > +++ b/xen/include/xsm/xsm.h > @@ -38,7 +38,6 @@ enum xsm_default { > XSM_DM_PRIV, /* Device model can perform on its target domain */ > XSM_TARGET, /* Can perform on self or your target domain */ > XSM_PRIV, /* Privileged - normally restricted to dom0 */ > - XSM_XS_PRIV, /* Xenstore domain - can do some privileged operations */ > XSM_OTHER /* Something more complex */ > }; > typedef enum xsm_default xsm_default_t; >
>>> On 23.06.16 at 17:22, <marmarek@invisiblethingslab.com> wrote: > xen: allow XEN_DOMCTL_getdomaininfo for device model domains > > Allow device model domain to get info about its target domain. > It is used during PCI passthrough setup (xc_domain_memory_mapping > checks for guest being auto-translated). While it happens in stubdomain, > it failed, breaking PCI passthrough in such setup. If that's the route to go (which I'm not convinced of, as I'm not sure we won't need other xenstore domain special casing later on) I'd really like to ask you to mention the other broken case too, as described in my original patch (unless you found I was wrong with that). Jan
On Thu, Jun 23, 2016 at 09:37:09AM -0600, Jan Beulich wrote: > >>> On 23.06.16 at 17:22, <marmarek@invisiblethingslab.com> wrote: > > xen: allow XEN_DOMCTL_getdomaininfo for device model domains > > > > Allow device model domain to get info about its target domain. > > It is used during PCI passthrough setup (xc_domain_memory_mapping > > checks for guest being auto-translated). While it happens in stubdomain, > > it failed, breaking PCI passthrough in such setup. > > If that's the route to go (which I'm not convinced of, as I'm not sure > we won't need other xenstore domain special casing later on) I'd > really like to ask you to mention the other broken case too, as > described in my original patch (unless you found I was wrong with > that). So, maybe something like this: case XEN_DOMCTL_getdomaininfo: if ( current-domain->is_xenstore ) return xsm_default_action(XSM_XS_PRIV, current->domain, d);; return xsm_default_action(XSM_DM_PRIV, current->domain, d); In your patch (changing XSM_XS_PRIV semantic), you implicitly considered all domctls allowed for xenstore domain to be always a subset of those allowed for device model domain. For now this is true, but if this set is going to be extended in the future, your approach most likely will lead to an error.
On Thu, Jun 23, 2016 at 05:45:22PM +0200, Marek Marczykowski-Górecki wrote: > On Thu, Jun 23, 2016 at 09:37:09AM -0600, Jan Beulich wrote: > > >>> On 23.06.16 at 17:22, <marmarek@invisiblethingslab.com> wrote: > > > xen: allow XEN_DOMCTL_getdomaininfo for device model domains > > > > > > Allow device model domain to get info about its target domain. > > > It is used during PCI passthrough setup (xc_domain_memory_mapping > > > checks for guest being auto-translated). While it happens in stubdomain, > > > it failed, breaking PCI passthrough in such setup. > > > > If that's the route to go (which I'm not convinced of, as I'm not sure > > we won't need other xenstore domain special casing later on) I'd > > really like to ask you to mention the other broken case too, as > > described in my original patch (unless you found I was wrong with > > that). > > So, maybe something like this: > case XEN_DOMCTL_getdomaininfo: > if ( current-domain->is_xenstore ) > return xsm_default_action(XSM_XS_PRIV, current->domain, d);; > return xsm_default_action(XSM_DM_PRIV, current->domain, d); > > > In your patch (changing XSM_XS_PRIV semantic), you implicitly considered > all domctls allowed for xenstore domain to be always a subset of those > allowed for device model domain. For now this is true, but if this set > is going to be extended in the future, your approach most likely will > lead to an error. Hmm, but if xenstore domain will never be also device model domain, this probably change nothing...
>>> On 23.06.16 at 17:45, <marmarek@invisiblethingslab.com> wrote: > In your patch (changing XSM_XS_PRIV semantic), you implicitly considered > all domctls allowed for xenstore domain to be always a subset of those > allowed for device model domain. For now this is true, but if this set > is going to be extended in the future, your approach most likely will > lead to an error. I don't think so (and intentionally accepted that resulting behavior), but in the end only the future can prove this either way. Jan
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index 406cd18..2768861 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -71,10 +71,6 @@ static always_inline int xsm_default_action( 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; @@ -128,7 +124,9 @@ static XSM_INLINE int xsm_domctl(XSM_DEFAULT_ARG struct domain *d, int cmd) case XEN_DOMCTL_unbind_pt_irq: return xsm_default_action(XSM_DM_PRIV, current->domain, d); case XEN_DOMCTL_getdomaininfo: - return xsm_default_action(XSM_XS_PRIV, current->domain, d); + if ( current->domain->is_xenstore ) + return 0; + return xsm_default_action(XSM_DM_PRIV, current->domain, d); default: return xsm_default_action(XSM_PRIV, current->domain, d); } diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h index 0d525ec..09672e7 100644 --- a/xen/include/xsm/xsm.h +++ b/xen/include/xsm/xsm.h @@ -38,7 +38,6 @@ enum xsm_default { XSM_DM_PRIV, /* Device model can perform on its target domain */ XSM_TARGET, /* Can perform on self or your target domain */ XSM_PRIV, /* Privileged - normally restricted to dom0 */ - XSM_XS_PRIV, /* Xenstore domain - can do some privileged operations */ XSM_OTHER /* Something more complex */ }; typedef enum xsm_default xsm_default_t;