Re: [Xen-devel] PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall"

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.06.16 at 20:24, <dgdegra@xxxxxxxxxxxxx> wrote:
> On 06/22/2016 11:23 AM, Jan Beulich wrote:
>>>>> On 22.06.16 at 16:13, <marmarek@xxxxxxxxxxxxxxxxxxxxxx> wrote:
>>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
>>>>>>> On 22.06.16 at 15:03, <marmarek@xxxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxx>
---
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?

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;



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel