From mboxrd@z Thu Jan  1 00:00:00 1970
From: Tim Deegan <tim@xen.org>
Subject: Re: [PATCH V6 11/13] xen/vm_event: Relocate memop checks
Date: Thu, 12 Mar 2015 16:36:11 +0100
Message-ID: <20150312153611.GI22158@deinos.phlegethon.org>
References: <1424218303-11331-1-git-send-email-tamas.lengyel@zentific.com>
	<1424218303-11331-12-git-send-email-tamas.lengyel@zentific.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Content-Disposition: inline
In-Reply-To: <1424218303-11331-12-git-send-email-tamas.lengyel@zentific.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: Tamas K Lengyel <tamas.lengyel@zentific.com>
Cc: kevin.tian@intel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, steve@zentific.com, stefano.stabellini@eu.citrix.com, jun.nakajima@intel.com, eddie.dong@intel.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, andres@lagarcavilla.org, jbeulich@suse.com, rshriram@cs.ubc.ca, keir@xen.org, dgdegra@tycho.nsa.gov, yanghy@cn.fujitsu.com
List-Id: xen-devel@lists.xenproject.org

Hi,

At 01:11 +0100 on 18 Feb (1424218301), Tamas K Lengyel wrote:
> -int mem_paging_memop(struct domain *d, xen_mem_paging_op_t *mpo)
> +int mem_paging_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_paging_op_t) arg)
>  {
> -    int rc = -ENODEV;
> -    if ( unlikely(!d->vm_event->paging.ring_page) )
> +    int rc;
> +    xen_mem_paging_op_t mpo;
> +    struct domain *d;
> +    bool_t copyback = 0;
> +
> +    rc = -EFAULT;
> +    if ( copy_from_guest(&mpo, arg, 1) )
>          return rc;

ISTR Jan suggested that you just 'return -EFAULT' here since you won't
be reusing the rc.

> -    switch( mpo->op )
> +    rc = rcu_lock_live_remote_domain_by_id(mpo.domain, &d);
> +    if ( rc )
> +        goto out;

This one should be a return too; you only need the goto out if this
succeeded.

> +    rc = xsm_vm_event_op(XSM_DM_PRIV, d, XENMEM_paging_op);
> +    if ( rc )
> +        goto out;
> +
> +    rc = -ENODEV;
> +    if ( unlikely(!d->vm_event->paging.ring_page) )
> +        goto out;
> +
> +    switch( mpo.op )
>      {
>      case XENMEM_paging_op_nominate:
> -        rc = p2m_mem_paging_nominate(d, mpo->gfn);
> +        rc = p2m_mem_paging_nominate(d, mpo.gfn);
>          break;
>  
>      case XENMEM_paging_op_evict:
> -        rc = p2m_mem_paging_evict(d, mpo->gfn);
> +        rc = p2m_mem_paging_evict(d, mpo.gfn);
>          break;
>  
>      case XENMEM_paging_op_prep:
> -        rc = p2m_mem_paging_prep(d, mpo->gfn, mpo->buffer);
> +    {

I don't think these braces are needed.

[...]
>      /* Only HAP is supported */
> +    rc = -ENODEV;
>      if ( !hap_enabled(d) || !d->arch.hvm_domain.mem_sharing_enabled )
> -         return -ENODEV;
> +         rc = -ENODEV;
>

You need a 'goto out' here, or the rc gets overwritten immediately.

Cheers,

Tim.

> -    switch(mec->op)
> +    rc = xsm_vm_event_op(XSM_DM_PRIV, d, XENMEM_sharing_op);
> +    if ( rc )
> +        goto out;