|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v1 2/5] xen/xsplice: Hypervisor implementation of XEN_XSPLICE_op
>>> On 16.09.15 at 23:01, <konrad.wilk@xxxxxxxxxx> wrote:
> --- /dev/null
> +++ b/xen/common/xsplice.c
> @@ -0,0 +1,442 @@
> +/*
> + * xSplice - Copyright Oracle Corp. Inc 2015.
> + *
> + * Author: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> + */
> +
> +#include <xen/smp.h>
> +#include <xen/keyhandler.h>
> +#include <xen/spinlock.h>
> +#include <xen/mm.h>
> +#include <xen/list.h>
> +#include <xen/guest_access.h>
> +#include <xen/stdbool.h>
No. This header should only be used in a very limited set up cases
(namely source code shared with the tools). Looks like we accumulated
a few other violations.
> +struct payload {
> + char *id; /* Name of it. Past this structure. */
> + ssize_t id_len; /* Length of the name. */
> +
> + uint8_t *raw; /* Pointer to Elf file. Past 'id'*/
> + ssize_t raw_len; /* Size of 'raw'. */
> +
> + int32_t status; /* XSPLICE_STATUS_* or Exx type value. */
> + int32_t old_status; /* XSPLICE_STATUS_* or Exx type value. */
> +
> + struct spinlock cmd_lock; /* Lock against the action. */
> + uint32_t cmd; /* Action request. XSPLICE_ACTION_* */
> +
> + /* Boring things below: */
> + struct list_head list; /* Linked to 'payload_list'. */
> + ssize_t len; /* This structure + raw_len + id_len + 1. */
> +
> + struct tasklet tasklet;
char name[]; here would avoid some casting further down.
> +static const char *status2str(int64_t status)
> +{
> +#define STATUS(x) [XSPLICE_STATUS_##x] = #x
> + static const char *const names[] = {
> + STATUS(LOADED),
> + STATUS(PROGRESS),
> + STATUS(CHECKED),
> + STATUS(APPLIED),
> + STATUS(REVERTED),
> + };
This array will grow unreasonably once we gain a few more
XSPLICE_STATUS_* values (which are bits masks, not enum like
values).
> +int find_payload(xen_xsplice_id_t *id, bool_t need_lock, struct payload **f)
static?
> +{
> + struct payload *data;
> + XEN_GUEST_HANDLE_PARAM(char) str;
> + char name[XEN_XSPLICE_ID_SIZE]; /* 128 bytes on stack. Perhaps kzalloc?
> */
I don't think 128 bytes are a significant problem, unless this could
be run in the context of an interrupt.
> +static int xsplice_upload(xen_sysctl_xsplice_upload_t *upload)
> +{
> + struct payload *data = NULL;
> + int rc;
> + ssize_t len;
> +
> + rc = verify_payload(upload);
> + if ( rc )
> + return rc;
> +
> + rc = find_payload(&upload->id, true, &data);
> + if ( rc == 0 /* Found. */ )
> + return -EEXIST;
> +
> + if ( rc != -ENOENT )
> + return rc;
> +
> + /*
> + * Compute the size of the structures which need to be verified.
> + * The 1 is for the extra \0 in case name does not have it.
> + */
> + len = sizeof(*data) + upload->id.size + 1 + upload->size;
> + data = alloc_xenheap_pages(get_order_from_bytes(len), 0);
> + if ( !data )
> + return -ENOMEM;
> +
> + memset(data, 0, len);
> + data->len = len;
> +
> + /* At the end of structure we put the name. */
> + data->id = (char *)data + sizeof(*data);
> + data->id_len = upload->id.size;
> + /* And after the name + \0 we stick the raw ELF data. */
> + data->raw = (uint8_t *)data + sizeof(*data) + data->id_len + 1;
Please use data->id here to shorten the expression.
> +static int xsplice_list(xen_sysctl_xsplice_list_t *list)
> +{
> + xen_xsplice_status_t status;
> + struct payload *data;
> + unsigned int idx = 0, i = 0;
> + int rc = 0;
> + unsigned int ver = payload_version;
Is it correct to latch this before taking the lock?
> + // TODO: Increase to a 64 or other value. Leave 4 for debug.
> + if ( list->nr > 4 )
> + return -E2BIG;
Command line driven?
> + if ( list->_pad != 0 )
> + return -EINVAL;
> +
> + if ( guest_handle_is_null(list->status) ||
> + guest_handle_is_null(list->id) ||
> + guest_handle_is_null(list->len) )
> + return -EINVAL;
Are these really useful?
> + if ( !guest_handle_okay(list->status, sizeof(status) * list->nr) ||
> + !guest_handle_okay(list->id, XEN_XSPLICE_ID_SIZE * list->nr) ||
> + !guest_handle_okay(list->len, sizeof(uint32_t) * list->nr) )
> + return -EINVAL;
> +
> + spin_lock(&payload_list_lock);
> + if ( list->idx > payload_cnt )
> + {
> + spin_unlock(&payload_list_lock);
> + return -EINVAL;
> + }
> +
> + status._pad = 0; /* No stack leaking. */
> + list_for_each_entry( data, &payload_list, list )
> + {
> + uint32_t len;
> +
> + if ( list->idx > i++ )
> + continue;
> +
> + status.status = data->status;
> + len = data->id_len;
> +
> + /* N.B. 'idx' != 'i'. */
> + if ( copy_to_guest_offset(list->id, idx * XEN_XSPLICE_ID_SIZE,
> + data->id, len) ||
> + copy_to_guest_offset(list->len, idx, &len, 1) ||
> + copy_to_guest_offset(list->status, idx, &status, 1) )
> + {
> + rc = -EFAULT;
> + break;
> + }
> + idx ++;
> + if ( hypercall_preempt_check() || (idx + 1 > list->nr) )
> + {
> + break;
> + }
Pointless braces. (I won't make specific note of the many other
coding style issues.)
> +static int xsplice_action(xen_sysctl_xsplice_action_t *action)
> +{
> + struct payload *data;
> + int rc;
> +
> + if ( action->_pad != 0 )
> + return -EINVAL;
> +
> + rc = verify_id(&action->id);
> + if ( rc )
> + return rc;
> +
> + spin_lock(&payload_list_lock);
> + rc = find_payload(&action->id, false /* we are holding the lock. */,
> &data);
> + if ( rc )
> + goto out;
> +
> + if ( action->cmd != XSPLICE_ACTION_UNLOAD )
> + spin_lock(&data->cmd_lock);
Wouldn't it be better to drop the lock in the specific case below,
after having done the ->status checks?
> + switch ( action->cmd )
> + {
> + case XSPLICE_ACTION_CHECK:
> + if ( ( data->status == XSPLICE_STATUS_LOADED ) )
> + {
> + data->old_status = data->status;
> + data->status = XSPLICE_STATUS_PROGRESS;
> + data->cmd = action->cmd;
> + tasklet_schedule(&data->tasklet);
> + rc = 0;
> + } else if ( data->status == XSPLICE_STATUS_CHECKED )
> + {
> + rc = 0;
> + }
> + break;
> + case XSPLICE_ACTION_UNLOAD:
> + if ( ( data->status == XSPLICE_STATUS_REVERTED ) ||
> + ( data->status == XSPLICE_STATUS_LOADED ) ||
> + ( data->status == XSPLICE_STATUS_CHECKED ) )
> + {
> + __free_payload(data);
> + /* No touching 'data' from here on! */
> + rc = 0;
> + }
> + break;
> + case XSPLICE_ACTION_REVERT:
> + if ( data->status == XSPLICE_STATUS_APPLIED )
> + {
> + data->old_status = data->status;
> + data->status = XSPLICE_STATUS_PROGRESS;
> + data->cmd = action->cmd;
> + rc = 0;
> + /* TODO: Tasklet is not good for this. We need a different
> vehicle. */
> + tasklet_schedule(&data->tasklet);
> + }
> + break;
> + case XSPLICE_ACTION_APPLY:
> + if ( ( data->status == XSPLICE_STATUS_CHECKED ) ||
> + ( data->status == XSPLICE_STATUS_REVERTED ))
> + {
> + data->old_status = data->status;
> + data->status = XSPLICE_STATUS_PROGRESS;
> + data->cmd = action->cmd;
> + rc = 0;
> + /* TODO: Tasklet is not good for this. We need a different
> vehicle. */
> + tasklet_schedule(&data->tasklet);
> + }
> + break;
Looking at all of these ->status checks - why do XSPLICE_STATUS_*
need to be bit masks rather then enum like values?
> + default:
> + rc = -ENOSYS;
EOPNOTSUPP or EINVAL or ..., but not ENOSYS.
> +int xsplice_control(xen_sysctl_xsplice_op_t *xsplice)
> +{
> + int rc;
> +
> + switch ( xsplice->cmd )
> + {
> + case XEN_SYSCTL_XSPLICE_UPLOAD:
> + rc = xsplice_upload(&xsplice->u.upload);
> + break;
> + case XEN_SYSCTL_XSPLICE_GET:
> + rc = xsplice_get(&xsplice->u.get);
> + break;
> + case XEN_SYSCTL_XSPLICE_LIST:
> + rc = xsplice_list(&xsplice->u.list);
> + break;
> + case XEN_SYSCTL_XSPLICE_ACTION:
> + rc = xsplice_action(&xsplice->u.action);
> + break;
> + default:
> + rc = -ENOSYS;
Same here.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |