Re: [Xen-devel] [PATCH v2] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

2017-06-26 Thread Jan Beulich 
>>> Daniel De Graaf  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 
>> ---
>> 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


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

2017-06-23 Thread Daniel De Graaf 

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 
---
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.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v2] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

2017-06-23 Thread Jan Beulich 
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 
---
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.

--- 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,
-);
-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 ==