Re: [Xen-devel] [PATCH v4 12/34] xen/xsplice: Hypervisor implementation of XEN_XSPLICE_op

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.03.16 at 18:56, <konrad.wilk@xxxxxxxxxx> wrote:
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -168,4 +168,15 @@ config SCHED_DEFAULT
>  
>  endmenu
>  
> +# Enable/Disable xsplice support
> +config XSPLICE
> +     bool "xSplice live patching support"
> +     default y

Isn't it a little early in the series to default this to on?

And then of course the EXPERT question comes up again. No
matter that IanC is no longer around to help with the
argumentation, the point he has been making about too many
flavors ending up in the wild continues to apply.

> @@ -460,6 +461,12 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>          ret = tmem_control(&op->u.tmem_op);
>          break;
>  
> +    case XEN_SYSCTL_xsplice_op:
> +        ret = xsplice_op(&op->u.xsplice);
> +        if ( ret != -ENOSYS )
> +            copyback = 1;
> +        break;

Why is ENOSYS special here, but not e.g. EOPNOTSUPP?

> +struct payload {
> +    uint32_t state;                      /* One of the XSPLICE_STATE_*. */
> +    int32_t rc;                          /* 0 or -XEN_EXX. */
> +    struct list_head list;               /* Linked to 'payload_list'. */
> +    char name[XEN_XSPLICE_NAME_SIZE + 1];/* Name of it. */

Could I talk you into reducing XEN_XSPLICE_NAME_SIZE to 127,
to avoid needless padding in places like this one?

> +static int verify_name(const xen_xsplice_name_t *name)
> +{
> +    if ( name->size == 0 || name->size > XEN_XSPLICE_NAME_SIZE )
> +        return -EINVAL;
> +
> +    if ( name->pad[0] || name->pad[1] || name->pad[2] )

I'd like to ask for consistency here: Either always use == 0 / != 0,
or always omit the latter and use ! in place of the former.

> +static int verify_payload(const xen_sysctl_xsplice_upload_t *upload)
> +{
> +    if ( verify_name(&upload->name) )
> +        return -EINVAL;
> +
> +    if ( upload->size == 0 )
> +        return -EINVAL;
> +
> +    if ( !guest_handle_okay(upload->payload, upload->size) )

Careful here - upload->size is uint64_t, yet array_access_ok() makes
assumptions on not too large a size getting passed. I.e. I think you
want to apply an upper bound to the size right here - for example, it
can't reasonably be bigger than XEN_VIRT_END - XEN_VIRT_START
if I remember correctly how you intend to place those payloads.

> +static int find_payload(const xen_xsplice_name_t *name, struct payload **f)

Perhaps neater to use the xen/err.h constructs here instead
of indirection?

> +{
> +    struct payload *data;
> +    XEN_GUEST_HANDLE_PARAM(char) str;
> +    char n[XEN_XSPLICE_NAME_SIZE + 1] = { 0 };

This pointlessly zeroes the entire array. Just set str[name->size]
to zero after the copy-in.

> +    int rc = -EINVAL;

Pointless initializer.

> +    rc = verify_name(name);
> +    if ( rc )
> +        return rc;
> +
> +    str = guest_handle_cast(name->name, char);

Why do you need a cast here?

> +    if ( copy_from_guest(n, str, name->size) )

You validated the address range already, so __copy_from_guest()
will be just fine and more efficient.

> +        return -EFAULT;
> +
> +    spin_lock_recursive(&payload_lock);

Why do you need a recursive lock here? I think something like this
should be reasoned about in the commit message.

> +/*
> + * We MUST be holding the payload_lock spinlock.
> + */

Single line comment (but kind of redundant with ...

> +static void free_payload(struct payload *data)
> +{
> +    ASSERT(spin_is_locked(&payload_lock));

... this anyway).

> +static int xsplice_upload(xen_sysctl_xsplice_upload_t *upload)
> +{
> +    struct payload *data = NULL;

Pointless initializer.

> +    void *raw_data = NULL;
> +    int rc;
> +
> +    rc = verify_payload(upload);
> +    if ( rc )
> +        return rc;
> +
> +    rc = find_payload(&upload->name, &data);
> +    if ( rc == 0 /* Found. */ )
> +        return -EEXIST;
> +
> +    if ( rc != -ENOENT )
> +        return rc;
> +
> +    data = xzalloc(struct payload);
> +    if ( !data )
> +        return -ENOMEM;
> +
> +    rc = -EFAULT;
> +    if ( copy_from_guest(data->name, upload->name.name, upload->name.size) )

__copy_from_guest()

> +        goto out;
> +
> +    rc = -ENOMEM;
> +    raw_data = vzalloc(upload->size);

vmalloc()

> +    if ( !raw_data )
> +        goto out;
> +
> +    rc = -EFAULT;
> +    if ( copy_from_guest(raw_data, upload->payload, upload->size) )

__copy_from_guest()

> +        goto out;
> +
> +    data->state = XSPLICE_STATE_CHECKED;
> +    data->rc = 0;

This is redundant with the xzalloc() above.

> +    INIT_LIST_HEAD(&data->list);
> +
> +    spin_lock_recursive(&payload_lock);
> +    list_add_tail(&data->list, &payload_list);
> +    payload_cnt++;
> +    payload_version++;
> +    spin_unlock_recursive(&payload_lock);
> +
> + out:
> +    vfree(raw_data);

By here you allocated and filled raw_data. And now you
unconditionally free it. What is that good for?

> +    if ( rc )
> +    {
> +        xfree(data);
> +    }

The use of braces here is inconsistent with all of the rest of this
function.

> +static int xsplice_get(xen_sysctl_xsplice_get_t *get)
> +{
> +    struct payload *data;
> +    int rc;
> +
> +    rc = verify_name(&get->name);
> +    if ( rc )
> +        return rc;
> +
> +    rc = find_payload(&get->name, &data);
> +    if ( rc )
> +        return rc;
> +
> +    get->status.state = data->state;
> +    get->status.rc = data->rc;

What guarantees that data didn't get freed by the time you get here?

> +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;
> +
> +    if ( list->nr > 1024 )
> +        return -E2BIG;
> +
> +    if ( list->pad != 0 )
> +        return -EINVAL;
> +
> +    if ( !guest_handle_okay(list->status, sizeof(status) * list->nr) ||
> +         !guest_handle_okay(list->name, XEN_XSPLICE_NAME_SIZE * list->nr) ||
> +         !guest_handle_okay(list->len, sizeof(uint32_t) * list->nr) )

guest_handle_okay() already takes into account the element size,
i.e. it's only the middle one which needs to do any multiplication.

> +        return -EINVAL;
> +
> +    spin_lock_recursive(&payload_lock);
> +    if ( list->idx > payload_cnt || !list->nr )

The list->nr check could move up outside the locked region (e.g.
merge with the pad field check).

> +    {
> +        spin_unlock_recursive(&payload_lock);
> +        return -EINVAL;
> +    }
> +
> +    list_for_each_entry( data, &payload_list, list )

Aren't you lacking a list->version check prior to entering this loop
(which would then mean you don't need to store it below, but only
on the error path from that check)?

> +    {
> +        uint32_t len;
> +
> +        if ( list->idx > i++ )
> +            continue;
> +
> +        status.state = data->state;
> +        status.rc = data->rc;
> +        len = strlen(data->name);
> +
> +        /* N.B. 'idx' != 'i'. */
> +        if ( __copy_to_guest_offset(list->name, idx * XEN_XSPLICE_NAME_SIZE,
> +                                    data->name, len) ||
> +             __copy_to_guest_offset(list->len, idx, &len, 1) ||

You're not coping the NUL terminator here, which makes the result
more cumbersome to consume by the caller. Perhaps
XEN_XSPLICE_NAME_SIZE should remain to be 128 (other than
suggested above), but be specified to include the terminator?

> +             __copy_to_guest_offset(list->status, idx, &status, 1) )
> +        {
> +            rc = -EFAULT;
> +            break;
> +        }
> +
> +        idx++;
> +
> +        if ( hypercall_preempt_check() || (idx + 1 > list->nr) )

idx >= list->nr would seem easier to grok. Also the two should be
switched, as hypercall_preempt_check() is the more expensive of
the two checks.

> +static int xsplice_action(xen_sysctl_xsplice_action_t *action)
> +{
> +    struct payload *data;
> +    int rc;
> +
> +    rc = verify_name(&action->name);
> +    if ( rc )
> +        return rc;
> +
> +    spin_lock_recursive(&payload_lock);
> +    rc = find_payload(&action->name, &data);
> +    if ( rc )
> +        goto out;
> +
> +    switch ( action->cmd )
> +    {
> +    case XSPLICE_ACTION_CHECK:
> +        if ( data->state == XSPLICE_STATE_CHECKED )
> +        {
> +            /* No implementation yet. */
> +            data->state = XSPLICE_STATE_CHECKED;
> +            data->rc = 0;
> +            rc = 0;

rc is zero already.

> +        }
> +        break;
> +
> +    case XSPLICE_ACTION_UNLOAD:
> +        if ( data->state == XSPLICE_STATE_CHECKED )
> +        {
> +            free_payload(data);
> +            /* No touching 'data' from here on! */

Poison the pointer to make sure?

> +            rc = 0;
> +        }
> +        break;
> +
> +    case XSPLICE_ACTION_REVERT:
> +        if ( data->state == XSPLICE_STATE_APPLIED )
> +        {
> +            /* No implementation yet. */
> +            data->state = XSPLICE_STATE_CHECKED;
> +            data->rc = 0;
> +            rc = 0;
> +        }
> +        break;
> +
> +    case XSPLICE_ACTION_APPLY:
> +        if ( (data->state == XSPLICE_STATE_CHECKED) )

Stray parentheses.

> +static void xsplice_printall(unsigned char key)
> +{
> +    struct payload *data;
> +
> +    spin_lock_recursive(&payload_lock);

I think this would better be a try-lock, bailing if the acquire failed.

Jan

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