Re: [Xen-devel] [PATCH] domctl: fix IRQ permission granting/revocation
On 12/11/2014 06:44 AM, Jan Beulich wrote:
On 10.12.14 at 10:53, ian.campb...@eu.citrix.com wrote:
On Wed, 2014-12-10 at 08:07 +, Jan Beulich wrote:
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -981,18 +981,18 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
case XEN_DOMCTL_irq_permission:
{
-unsigned int pirq = op-u.irq_permission.pirq;
+unsigned int pirq = op-u.irq_permission.pirq, irq;
int allow = op-u.irq_permission.allow_access;
if ( pirq = d-nr_pirqs )
ret = -EINVAL;
-else if ( !pirq_access_permitted(current-domain, pirq) ||
+else if ( !(irq = pirq_access_permitted(current-domain, pirq)) ||
I find hiding an assignment inside the second condition in a chain of
if's to be rather obfuscated. Doing an assignment in a standalone if
statement is one thing, this is going to far IMHO.
Changed. My main intention was to avoid having to add another
break;.
Also, you range check pirq against d-nr_pirqs but then translate it
against current-domain, is that correct?
No, it's not. As much as xsm_irq_permission(XSM_HOOK, d, pirq, allow)
is bogus, yet it's not clear to me whether switching that to use the Xen
IRQ number is okay without any other changes. Daniel?
Jan
At the moment, this XSM hook does not inspect the PIRQ argument, so it
is safe to change it to use the Xen IRQ number. The XSM hooks currently
only inspect the IRQ at mapping time; with this change (and the prior
changes that fixed up the IRQ permissions rangesets), it may make sense
to either add a check here or move the check in order to catch the error
earlier.
--
Daniel De Graaf
National Security Agency
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] domctl: fix IRQ permission granting/revocation
On Wed, 2014-12-10 at 08:07 +, Jan Beulich wrote:
Commit 545607eb3c (x86: fix various issues with handling guest IRQs)
wasn't really consistent in one respect: The granting of access to an
IRQ shouldn't assume the pIRQ-IRQ translation to be the same in both
domains. In fact it is wrong to assume that a translation is already/
still in place at the time access is being granted/revoked.
Specifically you need to do the translation using the mapping of the
domain doing the granting, not the domain being granted too, correct?
It takes a little bit of thought to figure out which domain to check
here, it would be worth a sentence or two explaining why this is the
right one.
Signed-off-by: Jan Beulich jbeul...@suse.com
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -981,18 +981,18 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
case XEN_DOMCTL_irq_permission:
{
-unsigned int pirq = op-u.irq_permission.pirq;
+unsigned int pirq = op-u.irq_permission.pirq, irq;
int allow = op-u.irq_permission.allow_access;
if ( pirq = d-nr_pirqs )
ret = -EINVAL;
-else if ( !pirq_access_permitted(current-domain, pirq) ||
+else if ( !(irq = pirq_access_permitted(current-domain, pirq)) ||
I find hiding an assignment inside the second condition in a chain of
if's to be rather obfuscated. Doing an assignment in a standalone if
statement is one thing, this is going to far IMHO.
Also, you range check pirq against d-nr_pirqs but then translate it
against current-domain, is that correct?
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] domctl: fix IRQ permission granting/revocation
On 10.12.14 at 10:53, ian.campb...@eu.citrix.com wrote: On Wed, 2014-12-10 at 08:07 +, Jan Beulich wrote: Commit 545607eb3c (x86: fix various issues with handling guest IRQs) wasn't really consistent in one respect: The granting of access to an IRQ shouldn't assume the pIRQ-IRQ translation to be the same in both domains. In fact it is wrong to assume that a translation is already/ still in place at the time access is being granted/revoked. Specifically you need to do the translation using the mapping of the domain doing the granting, not the domain being granted too, correct? It takes a little bit of thought to figure out which domain to check here, it would be worth a sentence or two explaining why this is the right one. Would What is wanted is to translate the incoming pIRQ to an IRQ for the invoking domain (as the pIRQ is the only notion the invoking domain has of the IRQ), and grant the subject domain access to the resulting IRQ. make this more clear without being purely redundant with the code? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] domctl: fix IRQ permission granting/revocation
On 10.12.14 at 11:19, julien.gr...@linaro.org wrote: Hi Jan, On 10/12/2014 08:07, Jan Beulich wrote: Commit 545607eb3c (x86: fix various issues with handling guest IRQs) wasn't really consistent in one respect: The granting of access to an IRQ shouldn't assume the pIRQ-IRQ translation to be the same in both domains. In fact it is wrong to assume that a translation is already/ still in place at the time access is being granted/revoked. With the change to the interface, some part of libxl may misuse xc_domain_irq_permission. For instance in tools/libxl/libxl_create.c: 1178 ret = irq = 0 ? xc_physdev_map_pirq(CTX-xch, domid, irq, irq) We get the PIRQ of domain domid in irq. 1179: -EOVERFLOW; 1180 if (!ret) 1181 ret = xc_domain_irq_permission(CTX-xch, domid, irq, 1) Here, the PIRQ of the current domain should be passed. Fortunately, for this specific case, the PIRQs are the same. But this is confusing. Agreed, but I'd leave it to the tools maintainers to clean this up. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
