Re: [Xen-devel] [PATCH v10 1/9] xen: vnuma topology and subop hypercalls

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.09.14 at 06:21, <ufimtseva@xxxxxxxxx> wrote:
> +static struct vnuma_info *vnuma_init(const struct xen_domctl_vnuma *uinfo,
> +                                     const struct domain *d)
> +{
> +    unsigned int nr_vnodes;
> +    int i, ret = -EINVAL;

"i" really ought to be unsigned too.

> +    struct vnuma_info *info;
> +
> +    nr_vnodes = uinfo->nr_vnodes;
> +
> +    if ( nr_vnodes == 0 || nr_vnodes > uinfo->nr_vmemranges ||

The earlier question on the second of these checks stands.

> +         uinfo->nr_vcpus != d->max_vcpus || uinfo->pad != 0 )
> +        return ERR_PTR(ret);
> +
> +    info = vnuma_alloc(nr_vnodes, uinfo->nr_vmemranges, d->max_vcpus);
> +    if ( IS_ERR(info) )
> +        return NULL;

I think you'd be better off returning info here (see below).

> +    case XEN_DOMCTL_setvnumainfo:
> +    {
> +        struct vnuma_info *vnuma;
> +
> +        vnuma = vnuma_init(&op->u.vnuma, d);
> +        if ( IS_ERR(vnuma) )
> +        {
> +            ret = -PTR_ERR(vnuma);

The negation seems wrong.

> +            break;
> +        }
> +
> +        ASSERT(vnuma != NULL);

This will trigger if the allocation earlier on fails, and you have
vnuma_init() return NULL.

> +    case XENMEM_get_vnumainfo:
> +    {
> +        struct vnuma_topology_info topology;
> +        struct domain *d;
> +        unsigned int dom_vnodes, dom_vranges, dom_vcpus;
> +        struct vnuma_info tmp;
> +
> +        /*
> +         * Guest passes nr_vnodes, number of regions and nr_vcpus thus
> +         * we know how much memory guest has allocated.
> +         */
> +        if ( copy_from_guest(&topology, arg, 1 ))
> +            return -EFAULT;
> +
> +        if ( (d = rcu_lock_domain_by_any_id(topology.domid)) == NULL )
> +            return -ESRCH;
> +
> +        read_lock(&d->vnuma_rwlock);
> +
> +        if ( d->vnuma == NULL )
> +        {
> +            read_unlock(&d->vnuma_rwlock);
> +            rcu_unlock_domain(d);
> +            return -EOPNOTSUPP;
> +        }
> +
> +        dom_vnodes = d->vnuma->nr_vnodes;
> +        dom_vranges = d->vnuma->nr_vmemranges;
> +        dom_vcpus = d->max_vcpus;
> +
> +        /*
> +         * Copied from guest values may differ from domain vnuma config.
> +         * Check here guest parameters make sure we dont overflow.
> +         * Additionaly check padding.
> +         */
> +        if ( topology.nr_vnodes < dom_vnodes      ||
> +             topology.nr_vcpus < dom_vcpus        ||
> +             topology.nr_vmemranges < dom_vranges ||
> +             topology.pad != 0 )

This last one clearly is -EINVAL, not -ENOBUFS. And for simple error
handling the check could be done earlier on.

> +        {
> +            read_unlock(&d->vnuma_rwlock);
> +            rcu_unlock_domain(d);
> +
> +            topology.nr_vnodes = dom_vnodes;
> +            topology.nr_vcpus = dom_vcpus;
> +            topology.nr_vmemranges = dom_vranges;
> +
> +            /* Copy back needed values. */
> +             __copy_to_guest(arg, &topology, 1);
> +
> +            return -ENOBUFS;
> +        }
> +
> +        read_unlock(&d->vnuma_rwlock);
> +
> +        tmp.vdistance = xmalloc_array(unsigned int, dom_vnodes * dom_vnodes);
> +        tmp.vmemrange = xmalloc_array(vmemrange_t, dom_vranges);
> +        tmp.vcpu_to_vnode = xmalloc_array(unsigned int, dom_vcpus);
> +
> +        if ( tmp.vdistance == NULL || tmp.vmemrange == NULL ||
> +             tmp.vcpu_to_vnode == NULL )
> +        {
> +            rc = -ENOMEM;
> +            goto vnumainfo_out;
> +        }
> +
> +        /*
> +         * Check if vnuma info has changed and if the allocated arrays
> +         * are not big enough.
> +         */
> +        read_lock(&d->vnuma_rwlock);
> +
> +        if ( dom_vnodes < d->vnuma->nr_vnodes ||
> +             dom_vranges < d->vnuma->nr_vmemranges ||
> +             dom_vcpus < d->max_vcpus )
> +        {
> +            read_unlock(&d->vnuma_rwlock);
> +            rc = -EAGAIN;
> +            goto vnumainfo_out;
> +        }
> +
> +        dom_vnodes = d->vnuma->nr_vnodes;
> +        dom_vranges = d->vnuma->nr_vmemranges;
> +        dom_vcpus = d->max_vcpus;
> +
> +        memcpy(tmp.vmemrange, d->vnuma->vmemrange,
> +               sizeof(*d->vnuma->vmemrange) * dom_vranges);
> +        memcpy(tmp.vdistance, d->vnuma->vdistance,
> +               sizeof(*d->vnuma->vdistance) * dom_vnodes * dom_vnodes);
> +        memcpy(tmp.vcpu_to_vnode, d->vnuma->vcpu_to_vnode,
> +               sizeof(*d->vnuma->vcpu_to_vnode) * dom_vcpus);
> +
> +        read_unlock(&d->vnuma_rwlock);
> +
> +        if ( copy_to_guest(topology.vmemrange.h, tmp.vmemrange,
> +                           dom_vranges) != 0 )
> +            goto vnumainfo_out;
> +
> +        if ( copy_to_guest(topology.vdistance.h, tmp.vdistance,
> +                           dom_vnodes * dom_vnodes) != 0 )
> +            goto vnumainfo_out;
> +
> +        if ( copy_to_guest(topology.vcpu_to_vnode.h, tmp.vcpu_to_vnode,
> +                           dom_vcpus) != 0 )
> +            goto vnumainfo_out;
> +
> +        topology.nr_vnodes = dom_vnodes;
> +        topology.nr_vcpus = dom_vcpus;
> +        topology.nr_vmemranges = dom_vranges;
> +
> +        if ( __copy_to_guest(arg, &topology, 1) != 0 )
> +            rc = -EFAULT;
> +        else rc = 0;

Now that's not a conditional operator and not a single line. But it'll
be okay anyway if you split the "else ..." into two lines; I'm
just confused since you said you would do the suggested conversion.

Jan

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