From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <Ian.Campbell@eu.citrix.com>
Subject: Re: [PATCH] domctl: fix IRQ permission
	granting/revocation
Date: Wed, 10 Dec 2014 09:53:16 +0000
Message-ID: <1418205196.19809.34.camel@eu.citrix.com>
References: <54880D6B020000780004E6AA@mail.emea.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta14.messagelabs.com ([193.109.254.103])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <Ian.Campbell@citrix.com>) id 1XydxR-0004PS-7N
	for xen-devel@lists.xenproject.org; Wed, 10 Dec 2014 09:53:21 +0000
In-Reply-To: <54880D6B020000780004E6AA@mail.emea.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>, Tim Deegan <tim@xen.org>, Keir Fraser <keir@xen.org>, Ian Jackson <Ian.Jackson@eu.citrix.com>
List-Id: xen-devel@lists.xenproject.org

On Wed, 2014-12-10 at 08:07 +0000, 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 <jbeulich@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.