diff mbox

[v2] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

Message ID 594D492502000078001666B2@prv-mh.provo.novell.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jan Beulich June 23, 2017, 3 p.m. UTC 
So far callers of the libxc interface passed in a domain ID which was
then ignored in the hypervisor. Instead, make the hypervisor honor it
(accepting DOMID_INVALID to obtain original behavior), allowing to
query whether a device can be assigned to a particular domain.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Alter the semantics to check whether the device can be assigned to
    the passed in domain.
TBD: It's not clear to me whether the test-assign XSM hooks are still
     useful this way.

Comments

Daniel De Graaf June 23, 2017, 4:49 p.m. UTC | #1 
On 06/23/2017 11:00 AM, Jan Beulich wrote:
> So far callers of the libxc interface passed in a domain ID which was
> then ignored in the hypervisor. Instead, make the hypervisor honor it
> (accepting DOMID_INVALID to obtain original behavior), allowing to
> query whether a device can be assigned to a particular domain.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Alter the semantics to check whether the device can be assigned to
>      the passed in domain.
> TBD: It's not clear to me whether the test-assign XSM hooks are still
>       useful this way.

As long as the only user of this hypercall is the device assignment
path, I would remove the XSM hook for test_assign and use the
assign_device hook for both test and actual.  That way, if XSM is
actually preventing the assignment, you will get the same AVC denials as
if you tried without doing the test first.  The hook should just skip the
two checks relating to (d) if it is null.

If this test hypercall were to be used as part of a query (such as
populating a list of assignable devices by trying all PCI devices listed
via sysfs), then it would make sense to have either a different hook or
a flag in the XSM hook to silence the audit messages since you aren't
actually trying to do the assignment yet.  That change will make the
cause of the "can't assign" error harder to diagnose, however, so unless
it's being used like that I don't think it's a good idea.
Jan Beulich June 26, 2017, 6:17 a.m. UTC | #2 
>>> Daniel De Graaf <dgdegra@tycho.nsa.gov> 06/23/17 6:49 PM >>>
>On 06/23/2017 11:00 AM, Jan Beulich wrote:
>> So far callers of the libxc interface passed in a domain ID which was
>> then ignored in the hypervisor. Instead, make the hypervisor honor it
>> (accepting DOMID_INVALID to obtain original behavior), allowing to
>> query whether a device can be assigned to a particular domain.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v2: Alter the semantics to check whether the device can be assigned to
>>      the passed in domain.
>> TBD: It's not clear to me whether the test-assign XSM hooks are still
>>       useful this way.
>
>As long as the only user of this hypercall is the device assignment
>path, I would remove the XSM hook for test_assign and use the
>assign_device hook for both test and actual.  That way, if XSM is
>actually preventing the assignment, you will get the same AVC denials as
>if you tried without doing the test first.  The hook should just skip the
>two checks relating to (d) if it is null.
>
>If this test hypercall were to be used as part of a query (such as
>populating a list of assignable devices by trying all PCI devices listed
>via sysfs), then it would make sense to have either a different hook or
>a flag in the XSM hook to silence the audit messages since you aren't
>actually trying to do the assignment yet.  That change will make the
>cause of the "can't assign" error harder to diagnose, however, so unless
>it's being used like that I don't think it's a good idea.

Well, that last aspect is what I'm currently retaining with the DOMID_INVALID
behavior, hence why I've wired that case to the test-assign hook. If we were
to remove that hook, then that special case should go away altogether imo
(completely doing away with original behavior). Anyway, let's first see what
others think ...

Jan
diff mbox

Patch

--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -391,11 +391,15 @@  long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
 
     switch ( op->cmd )
     {
-    case XEN_DOMCTL_createdomain:
     case XEN_DOMCTL_test_assign_device:
+        if ( op->domain == DOMID_INVALID )
+        {
+    case XEN_DOMCTL_createdomain:
     case XEN_DOMCTL_gdbsx_guestmemio:
-        d = NULL;
-        break;
+            d = NULL;
+            break;
+        }
+        /* fall through */
     default:
         d = rcu_lock_domain_by_id(op->domain);
         if ( !d && op->cmd != XEN_DOMCTL_getdomaininfo )
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -143,12 +143,15 @@  int iommu_do_dt_domctl(struct xen_domctl
     switch ( domctl->cmd )
     {
     case XEN_DOMCTL_assign_device:
+        ASSERT(d);
+        /* fall through */
+    case XEN_DOMCTL_test_assign_device:
         ret = -ENODEV;
         if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
             break;
 
         ret = -EINVAL;
-        if ( d->is_dying || domctl->u.assign_device.flags )
+        if ( (d && d->is_dying) || domctl->u.assign_device.flags )
             break;
 
         ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
@@ -157,10 +160,22 @@  int iommu_do_dt_domctl(struct xen_domctl
         if ( ret )
             break;
 
-        ret = xsm_assign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev));
+        ret = d ? xsm_assign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev))
+                : xsm_test_assign_dtdevice(XSM_HOOK, dt_node_full_name(dev));
         if ( ret )
             break;
 
+        if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
+        {
+            if ( iommu_dt_device_is_assigned(dev) )
+            {
+                printk(XENLOG_G_ERR "%s already assigned.\n",
+                       dt_node_full_name(dev));
+                ret = -EINVAL;
+            }
+            break;
+        }
+
         ret = iommu_assign_dt_device(d, dev);
 
         if ( ret )
@@ -194,33 +209,6 @@  int iommu_do_dt_domctl(struct xen_domctl
                    dt_node_full_name(dev), d->domain_id, ret);
         break;
 
-    case XEN_DOMCTL_test_assign_device:
-        ret = -ENODEV;
-        if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
-            break;
-
-        ret = -EINVAL;
-        if ( domctl->u.assign_device.flags )
-            break;
-
-        ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
-                                    domctl->u.assign_device.u.dt.size,
-                                    &dev);
-        if ( ret )
-            break;
-
-        ret = xsm_test_assign_dtdevice(XSM_HOOK, dt_node_full_name(dev));
-        if ( ret )
-            break;
-
-        if ( iommu_dt_device_is_assigned(dev) )
-        {
-            printk(XENLOG_G_ERR "%s already assigned.\n",
-                   dt_node_full_name(dev));
-            ret = -EINVAL;
-        }
-        break;
-
     default:
         ret = -ENOSYS;
         break;
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1579,35 +1579,10 @@  int iommu_do_pci_domctl(
     }
     break;
 
-    case XEN_DOMCTL_test_assign_device:
-        ret = -ENODEV;
-        if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_PCI )
-            break;
-
-        ret = -EINVAL;
-        if ( domctl->u.assign_device.flags )
-            break;
-
-        machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf;
-
-        ret = xsm_test_assign_device(XSM_HOOK, machine_sbdf);
-        if ( ret )
-            break;
-
-        seg = machine_sbdf >> 16;
-        bus = PCI_BUS(machine_sbdf);
-        devfn = PCI_DEVFN2(machine_sbdf);
-
-        if ( device_assigned(seg, bus, devfn) )
-        {
-            printk(XENLOG_G_INFO
-                   "%04x:%02x:%02x.%u already assigned, or non-existent\n",
-                   seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
-            ret = -EINVAL;
-        }
-        break;
-
     case XEN_DOMCTL_assign_device:
+        ASSERT(d);
+        /* fall through */
+    case XEN_DOMCTL_test_assign_device:
         /* Don't support self-assignment of devices. */
         if ( d == current->domain )
         {
@@ -1621,12 +1596,15 @@  int iommu_do_pci_domctl(
 
         ret = -EINVAL;
         flags = domctl->u.assign_device.flags;
-        if ( d->is_dying || (flags & ~XEN_DOMCTL_DEV_RDM_RELAXED) )
+        if ( domctl->cmd == XEN_DOMCTL_assign_device
+             ? d->is_dying || (flags & ~XEN_DOMCTL_DEV_RDM_RELAXED)
+             : flags )
             break;
 
         machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf;
 
-        ret = xsm_assign_device(XSM_HOOK, d, machine_sbdf);
+        ret = d ? xsm_assign_device(XSM_HOOK, d, machine_sbdf)
+                : xsm_test_assign_device(XSM_HOOK, machine_sbdf);
         if ( ret )
             break;
 
@@ -1634,8 +1612,20 @@  int iommu_do_pci_domctl(
         bus = PCI_BUS(machine_sbdf);
         devfn = PCI_DEVFN2(machine_sbdf);
 
-        ret = device_assigned(seg, bus, devfn) ?:
-              assign_device(d, seg, bus, devfn, flags);
+        ret = device_assigned(seg, bus, devfn);
+        if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
+        {
+            if ( ret )
+            {
+                printk(XENLOG_G_INFO
+                       "%04x:%02x:%02x.%u already assigned, or non-existent\n",
+                       seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
+                ret = -EINVAL;
+            }
+            break;
+        }
+        if ( !ret )
+            ret = assign_device(d, seg, bus, devfn, flags);
         if ( ret == -ERESTART )
             ret = hypercall_create_continuation(__HYPERVISOR_domctl,
                                                 "h", u_domctl);
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -506,7 +506,11 @@  DEFINE_XEN_GUEST_HANDLE(xen_domctl_sendt
 
 /* Assign a device to a guest. Sets up IOMMU structures. */
 /* XEN_DOMCTL_assign_device */
-/* XEN_DOMCTL_test_assign_device */
+/*
+ * XEN_DOMCTL_test_assign_device: Pass DOMID_INVALID to find out whether the
+ * given device is assigned to any DomU at all. Pass a specific domain ID to
+ * find out whether the given device can be assigned to that domain.
+ */
 /*
  * XEN_DOMCTL_deassign_device: The behavior of this DOMCTL differs
  * between the different type of device: