|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/7] xen: vNUMA support for PV guests
>>> On 17.10.13 at 00:37, Elena Ufimtseva <ufimtseva@xxxxxxxxx> wrote:
> Changes since RFC v2:
> - fixed code style;
Sadly not enough yet.
> @@ -871,6 +872,77 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
> u_domctl)
> }
> break;
>
> + case XEN_DOMCTL_setvnumainfo:
> + {
> + unsigned int i, j, nr_vnodes, dist_size;
> + unsigned int dist;
Why can't these all be together?
> +
Line with only blanks.
> + ret = -EFAULT;
> + dist = i = j = 0;
Why can't these be initializers to their declarators?
> +
> + nr_vnodes = op->u.vnuma.nr_vnodes;
> + if ( nr_vnodes == 0 )
> + goto err_dom;
> + dist_size = nr_vnodes * nr_vnodes;
Potential for overflow?
> +
> + d->vnuma.vdistance = xmalloc_bytes(
> + sizeof(*d->vnuma.vdistance) * dist_size);
> + d->vnuma.vnuma_memblks = xmalloc_bytes(
> + sizeof(*d->vnuma.vnuma_memblks) * nr_vnodes);
> + d->vnuma.vcpu_to_vnode = xmalloc_bytes(
> + sizeof(*d->vnuma.vcpu_to_vnode) * d->max_vcpus);
> + d->vnuma.vnode_to_pnode = xmalloc_bytes(
> + sizeof(*d->vnuma.vnode_to_pnode) * nr_vnodes);
Is there any reason why any of these really has to use
xmalloc_bytes() instead of xmalloc_array() (which takes care
of overflow conditions as well as type correctness for you)?
Further there's nothing preventing this call from being issued
twice for a given domain, yet you blindly overwrite the old
values (and thus leak them).
> +
> + if ( d->vnuma.vdistance == NULL || d->vnuma.vnuma_memblks == NULL ||
> + d->vnuma.vcpu_to_vnode == NULL ||
> + d->vnuma.vnode_to_pnode == NULL )
> + {
> + ret = -ENOMEM;
> + goto err_dom;
> + }
> +
> + d->vnuma.nr_vnodes = nr_vnodes;
Trailing blank.
Also I'd strongly recommend setting this field last, so that other
code won't risk getting confused if some of the copying below
fails - until all copying was successfully done, the domain should
look like not having any vNUMA info.
> + if ( !guest_handle_is_null(op->u.vnuma.vdistance) )
> + if ( unlikely(copy_from_guest(d->vnuma.vdistance,
Two if()-s like here should be combined into a single one.
> + op->u.vnuma.vdistance,
> + dist_size)) )
> + goto err_dom;
And if guest_handle_is_null(op->u.vnuma.vdistance)
d->vnuma.vdistance will remain uninitialized - is that not a
problem?
(Both this and the preceding comment apply further down
again.)
> +
> + if ( !guest_handle_is_null(op->u.vnuma.vnuma_memblks) )
> + if ( unlikely(copy_from_guest(d->vnuma.vnuma_memblks,
> + op->u.vnuma.vnuma_memblks,
> + nr_vnodes)))
> + goto err_dom;
> +
> + if ( !guest_handle_is_null(op->u.vnuma.vcpu_to_vnode) )
> + if ( unlikely(copy_from_guest(d->vnuma.vcpu_to_vnode,
> + op->u.vnuma.vcpu_to_vnode,
> + d->max_vcpus)) )
> + goto err_dom;
> +
> + if ( !guest_handle_is_null(op->u.vnuma.vnode_to_pnode) )
> + {
> + if ( unlikely(copy_from_guest(d->vnuma.vnode_to_pnode,
> + op->u.vnuma.vnode_to_pnode,
> + nr_vnodes)) )
> + goto err_dom;
> +
> + }
> + else
> + for( i = 0; i < nr_vnodes; i++ )
Missing blank after "for".
> + d->vnuma.vnode_to_pnode[i] = NUMA_NO_NODE;
> + ret = 0;
> +err_dom:
This should be indented by one space. I also don't see why it's being
named the way it is (it's for one too unspecific and also completely
unrelated to anything "dom"ish).
> + if (ret != 0) {
if ( ret != 0 )
{
> + d->vnuma.nr_vnodes = 0;
> + xfree( d->vnuma.vdistance );
> + xfree( d->vnuma.vnuma_memblks );
> + xfree( d->vnuma.vnode_to_pnode );
Bogus blanks around function call arguments.
> @@ -732,7 +733,47 @@ long do_memory_op(unsigned long cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> rcu_unlock_domain(d);
>
> break;
> + case XENMEM_get_vnuma_info:
> + {
> + struct vnuma_topology_info mtopology;
> + struct domain *d;
> + unsigned int max_vcpus, nr_vnodes = 0;
Pretty pointless variables, and definitely a pointless initializer.
>
The blank line here should actually be retained above the case
label (and similarly a blank line should be there before the next
[default] one).
> + rc = -EINVAL;
> +
> + if( copy_from_guest(&mtopology, arg, 1) )
> + return -EFAULT;
> + if ( (d = rcu_lock_domain_by_any_id(mtopology.domid)) == NULL )
> + return -ESRCH;
> +
> + nr_vnodes = d->vnuma.nr_vnodes;
> + max_vcpus = d->max_vcpus;
> +
> + if ((nr_vnodes == 0) || (nr_vnodes > max_vcpus))
Starting here and continuing all the way to the end of the function
you're again lacking blanks inside the outermost parentheses.
Also, is the right side of the || really possible?
> + goto vnumaout;
This yields a -EINVAL return for something that isn't really a
bad function argument. Please use a more appropriate error
code (like -EOPNOTSUPP).
> +
> + mtopology.nr_vnodes = nr_vnodes;
So this is the only field you changed.
> +
> + if (copy_to_guest(arg , &mtopology, 1) != 0)
Hence you would be better off copying back just that one field.
And having used copy_from_guest() on the same guest memory
block above, using the __-prefixed variant here is correct and
(for consistency with other similar code) recommended.
> + goto vnumaout;
And from here on the error return value ought to be -EFAULT.
> +
> + if (copy_to_guest(mtopology.vnuma_memblks,
> + d->vnuma.vnuma_memblks,
> + nr_vnodes) != 0)
And here we see that the lack of initialization above _is_ a
(security) problem - you're possibly leaking hypervisor memory
contents to the guest here.
> + goto vnumaout;
> + if (copy_to_guest(mtopology.vdistance,
> + d->vnuma.vdistance,
> + nr_vnodes * nr_vnodes) != 0)
> + goto vnumaout;
> + if (copy_to_guest(mtopology.vcpu_to_vnode,
> + d->vnuma.vcpu_to_vnode,
> + max_vcpus) != 0)
> + goto vnumaout;
> + rc = 0;
> +vnumaout:
Again needs to be indented by one space.
> @@ -863,6 +864,17 @@ struct xen_domctl_set_max_evtchn {
> typedef struct xen_domctl_set_max_evtchn xen_domctl_set_max_evtchn_t;
> DEFINE_XEN_GUEST_HANDLE(xen_domctl_set_max_evtchn_t);
>
> +struct xen_domctl_vnuma {
> + uint16_t nr_vnodes;
Please insert explicit padding here. And if you want the structure
to be extensible without having to increment the domctl interface
version, you'd also require (and check!) the padding space to be
zero-initialized.
> +struct vnuma_memblk {
> + uint64_t start, end;
Too deep indentation.
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -4,6 +4,7 @@
>
> #include <public/xen.h>
> #include <asm/domain.h>
> +#include <public/memory.h>
Avoid such unnecessary dependencies if you can. And you can
here ...
> @@ -89,4 +90,14 @@ extern unsigned int xen_processor_pmbits;
>
> extern bool_t opt_dom0_vcpus_pin;
>
> +struct domain_vnuma_info {
> + uint16_t nr_vnodes;
> + uint *vdistance;
> + uint *vcpu_to_vnode;
> + uint *vnode_to_pnode;
> + vnuma_memblk_t *vnuma_memblks;
... by using struct vnuma_memblk here.
> --- /dev/null
> +++ b/xen/include/xen/vnuma.h
> @@ -0,0 +1,18 @@
> +#ifndef _VNUMA_H
> +#define _VNUMA_H
> +#include <public/memory.h>
> +
> +/* DEFINE_XEN_GUEST_HANDLE(vnuma_memblk_t); */
> +
> +struct vnuma_topology_info {
> + domid_t domid;
> + uint16_t nr_vnodes;
> + uint32_t _pad;
> + XEN_GUEST_HANDLE_64(uint) vdistance;
> + XEN_GUEST_HANDLE_64(uint) vcpu_to_vnode;
> + XEN_GUEST_HANDLE_64(vnuma_memblk_t) vnuma_memblks;
> +};
> +typedef struct vnuma_topology_info vnuma_topology_info_t;
> +DEFINE_XEN_GUEST_HANDLE(vnuma_topology_info_t);
This being the argument of XENMEM_get_vnuma_info, how can
this live in a non-public header? (I'm sure I had pointed this out
before.)
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |