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

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

On Tue, Feb 17, 2015 at 3:25 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 13.02.15 at 17:33, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>> -int mem_paging_memop(struct domain *d, xen_mem_paging_op_t *mpo)
>> +int mem_paging_memop(unsigned long cmd,
>> +                     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;
>
> Please don't make things more complicated than they need to be:
> You only use the -EFAULT once here, so no reason to assign it to
> rc up front.

This return will be a "goto out;" where the rcu is getting unlocked as well.

>
>> +
>> +    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 )
>
> There's an RCU lock you take right before this, which you now fail
> to drop here and below.

Ack.

>
>> +        return rc;
>> +
>> +    rc = -ENODEV;
>>      if ( unlikely(!d->vm_event->paging.ring_page) )
>>          return rc;
>
> Same comment as for the -EFAULT above.

Same as above, will be a goto out.

>
> Jan

Thanks,
Tamas

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