Re: [Xen-devel] [PATCH V5 10/12] xen/vm_event: Relocate memop checks

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Sat, Feb 14, 2015 at 12:20 AM, Tamas K Lengyel
<tamas.lengyel@xxxxxxxxxxxx> wrote:
> On Fri, Feb 13, 2015 at 10:23 PM, Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 13/02/15 16:33, Tamas K Lengyel wrote:
>>> The memop handler function for paging/sharing responsible for calling XSM
>>> doesn't really have anything to do with vm_event, thus in this patch we
>>> relocate it into mem_paging_memop and mem_sharing_memop. This has already
>>> been the approach in mem_access_memop, so in this patch we just make it
>>> consistent.
>>>
>>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
>>> ---
>>>  xen/arch/x86/mm/mem_paging.c      | 36 ++++++++++++++-----
>>>  xen/arch/x86/mm/mem_sharing.c     | 76 
>>> ++++++++++++++++++++++++++-------------
>>>  xen/arch/x86/x86_64/compat/mm.c   | 28 +++------------
>>>  xen/arch/x86/x86_64/mm.c          | 26 +++-----------
>>>  xen/common/vm_event.c             | 43 ----------------------
>>>  xen/include/asm-x86/mem_paging.h  |  7 +++-
>>>  xen/include/asm-x86/mem_sharing.h |  4 +--
>>>  xen/include/xen/vm_event.h        |  1 -
>>>  8 files changed, 97 insertions(+), 124 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/mm/mem_paging.c b/xen/arch/x86/mm/mem_paging.c
>>> index e63d8c1..4aee6b7 100644
>>> --- a/xen/arch/x86/mm/mem_paging.c
>>> +++ b/xen/arch/x86/mm/mem_paging.c
>>> @@ -21,28 +21,45 @@
>>>   */
>>>
>>>
>>> +#include <xen/guest_access.h>
>>>  #include <asm/p2m.h>
>>> -#include <xen/vm_event.h>
>>> +#include <xsm/xsm.h>
>>
>> Order of includes.
>>
>>>
>>> -
>>> -int mem_paging_memop(struct domain *d, xen_mem_paging_op_t *mpo)
>>> +int mem_paging_memop(unsigned long cmd,
>>
>> I don't believe cmd is a useful parameter to pass.  You know that its
>> value is XENMEM_paging_op by virtue of being in this function.
>>
>>> +                     XEN_GUEST_HANDLE_PARAM(xen_mem_paging_op_t) arg)
>>>  {
>>> -    int rc = -ENODEV;
>>> +    int rc;
>>> +    xen_mem_paging_op_t mpo;
>>> +    struct domain *d;
>>> +
>>> +    rc = -EFAULT;
>>> +    if ( copy_from_guest(&mpo, arg, 1) )
>>> +        return rc;
>>> +
>>> +    rc = rcu_lock_live_remote_domain_by_id(mpo.domain, &d);
>>> +    if ( rc )
>>> +        return rc;
>>> +
>>> +    rc = xsm_vm_event_op(XSM_DM_PRIV, d, XENMEM_paging_op);
>>> +    if ( rc )
>>> +        return rc;
>>> +
>>> +    rc = -ENODEV;
>>>      if ( unlikely(!d->vm_event->paging.ring_page) )
>>>          return rc;
>>>
>>> -    switch( mpo->op )
>>> +    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);
>>> +        rc = p2m_mem_paging_prep(d, mpo.gfn, mpo.buffer);
>>>          break;
>>>
>>>      default:
>>> @@ -50,6 +67,9 @@ int mem_paging_memop(struct domain *d, 
>>> xen_mem_paging_op_t *mpo)
>>>          break;
>>>      }
>>>
>>> +    if ( !rc && __copy_to_guest(arg, &mpo, 1) )
>>> +        rc = -EFAULT;
>>
>> Do any of the paging ops need to be copied back?  Nothing appears to
>> write into mpo in this function.  (The original code looks to be overly
>> pessimistic).
>
> I'm not sure if any of these actually copy back - I just tried to keep
> as much of the existing flow intact as possible while sanitizing the
> vm_event side of things. That is not to say mem_sharing and mem_paging
> couldn't use more scrutiny and optimizations, it's just a bit out of
> the scope of what this series is about.
>
>>
>>> +
>>>      return rc;
>>>  }
>>>
>>> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
>>> index 4959407..612ed89 100644
>>> --- a/xen/arch/x86/mm/mem_sharing.c
>>> +++ b/xen/arch/x86/mm/mem_sharing.c
>>> @@ -28,6 +28,7 @@
>>>  #include <xen/grant_table.h>
>>>  #include <xen/sched.h>
>>>  #include <xen/rcupdate.h>
>>> +#include <xen/guest_access.h>
>>>  #include <xen/vm_event.h>
>>>  #include <asm/page.h>
>>>  #include <asm/string.h>
>>> @@ -1293,30 +1294,52 @@ int relinquish_shared_pages(struct domain *d)
>>>      return rc;
>>>  }
>>>
>>> -int mem_sharing_memop(struct domain *d, xen_mem_sharing_op_t *mec)
>>> +int mem_sharing_memop(unsigned long cmd,
>>> +                      XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
>>>  {
>>> -    int rc = 0;
>>> +    int rc;
>>> +    xen_mem_sharing_op_t mso;
>>> +    struct domain *d;
>>> +
>>> +    rc = -EFAULT;
>>> +    if ( copy_from_guest(&mso, arg, 1) )
>>> +        return rc;
>>> +
>>> +    if ( mso.op == XENMEM_sharing_op_audit )
>>> +        return mem_sharing_audit();
>>> +
>>> +    rc = rcu_lock_live_remote_domain_by_id(mso.domain, &d);
>>> +    if ( rc )
>>> +        return rc;
>>>
>>>      /* Only HAP is supported */
>>>      if ( !hap_enabled(d) || !d->arch.hvm_domain.mem_sharing_enabled )
>>>           return -ENODEV;
>>>
>>> -    switch(mec->op)
>>> +    rc = xsm_vm_event_op(XSM_DM_PRIV, d, XENMEM_sharing_op);
>>> +    if ( rc )
>>> +        return rc;
>>> +
>>> +    rc = -ENODEV;
>>> +    if ( unlikely(!d->vm_event->share.ring_page) )
>>> +        return rc;
>>> +
>>> +    switch(mso.op)
>>
>> Style ( spaces )
>
> Ack.
>
>>
>>> @@ -1465,6 +1488,9 @@ int mem_sharing_memop(struct domain *d, 
>>> xen_mem_sharing_op_t *mec)
>>>              break;
>>>      }
>>>
>>> +    if ( !rc && __copy_to_guest(arg, &mso, 1) )
>>> +        return -EFAULT;
>>
>> Only certain subops need to copy information back.  It is common to have
>> a function-level bool_t copyback which relevant subops sets.
>
> If it's not a critical fix I would rather just keep it as it was in
> this series. It could be part of another cleanup series for mem_paging
> and mem_sharing.
>
> Thanks,
> Tamas

Nevermind, it's quite obvious that only XENMEM_paging_op_prep copies
stuff into the buffer so it's a no brainer. I'll add the fix.

Thanks,
Tamas

>
>>
>> ~Andrew
>>
>>> +
>>>      return rc;
>>>  }
>>
>>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel