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

[Jan Beulich]

>>> On 24.03.16 at 04:13,  wrote:
> On Wed, Mar 23, 2016 at 07:51:29AM -0600, Jan Beulich wrote:
>> >>> On 15.03.16 at 18:56,  wrote:
>> 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.
> 
> 'too many flavors'? As in different versions of Xen with or without
> these options enabled? 

Yes.

>> > +{
>> > +spin_unlock_recursive(_lock);
>> > +return -EINVAL;
>> > +}
>> > +
>> > +list_for_each_entry( data, _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)?
> 
> No. The toolstack has no idea of what the right version is on the
> first invocation. Which is OK since it gets fresh data (it is
> its first invocation).
> 
> On subsequent invocations we gleefuly populate up to
> min(payload_cnt, ->nr) of data even if the version the toolstack
> provided is different. The toolstack will have to decide to throw away
> the data and retry the hypercall; or print it out as is.

Makes sense, but doesn't really fit with this

+The caller provides:
+
+ * `version`. Version of the payload. Caller should re-use the field provided 
by
+the hypervisor. If the value differs the data is stale.

in the most recent patch 11.

> Here is the newly minted patch with your suggestions hopefully
> implemented to your liking!

I think this immediate providing of a partly next-version patch is
getting unwieldy: I just can't re-review several of these large
patches again every day. I'll look at the entire next version once
you've sent that out.

Jan


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

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

[Konrad Rzeszutek Wilk]

On Wed, Mar 23, 2016 at 07:51:29AM -0600, Jan Beulich wrote:
> >>> On 15.03.16 at 18:56,  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?

I am ambitious!
> 
> 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.

'too many flavors'? As in different versions of Xen with or without
these options enabled? 

.. snip..
> 
> > +static int find_payload(const xen_xsplice_name_t *name, struct payload **f)
> 
..snip..
> > +return -EFAULT;
> > +
> > +spin_lock_recursive(_lock);
> 
> Why do you need a recursive lock here? I think something like this
> should be reasoned about in the commit message.

The earlier version used an extra parameter (locked) to diffrenciate
whether to take a lock or not as the caller could have taken it.

Andrew didn't like it particularly and asked it to be recursive
so that we don't by accident mess up the locking.

.. snip..
> > +static int xsplice_upload(xen_sysctl_xsplice_upload_t *upload)

.. snip..
> > + out:
> > +vfree(raw_data);
> 
> By here you allocated and filled raw_data. And now you
> unconditionally free it. What is that good for?

Nothing. It was added as a placeholder - as the patch
titled "xsplice: Implement payload loading" is actually doing
useful things. I've moved the operations around raw_data into that
patch.

> > +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(_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).

I reworked this a bit. I made it so that if list->nr is 0 we would
populate list->nr=payload_count, list->version=payload_version.


> 
> > +{
> > +spin_unlock_recursive(_lock);
> > +return -EINVAL;
> > +}
> > +
> > +list_for_each_entry( data, _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)?

No. The toolstack has no idea of what the right version is on the
first invocation. Which is OK since it gets fresh data (it is
its first invocation).

On subsequent invocations we gleefuly populate up to
min(payload_cnt, ->nr) of data even if the version the toolstack
provided is different. The toolstack will have to decide to throw away
the data and retry the hypercall; or print it out as is.

> 
> > +{
> > +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, , 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?

Yes. Fixed that. It also needed a minor change in:
"libxc: Implementation of XEN_XSPLICE_op in libxc" to account for
strlen. (+1 to its result)


Here is the newly minted patch with your suggestions hopefully
implemented to your liking!

From 40f0e9fdb50935d4d3df608950313051a28f12b9 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk 
Date: Mon, 25 Jan 2016 10:51:22 -0500
Subject: [PATCH] xen/xsplice: Hypervisor implementation of XEN_XSPLICE_op

The implementation does not actually do any patching.

It just adds the framework for doing the hypercalls,
keeping track of ELF payloads, and the basic operations:
 - query which payloads exist,
 - query for 

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

[Jan Beulich]

>>> On 15.03.16 at 18:56,  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(>u.tmem_op);
>  break;
>  
> +case XEN_SYSCTL_xsplice_op:
> +ret = xsplice_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(>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(_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(_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(>name, );
> +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(>list);
> +
> +spin_lock_recursive(_lock);
> +list_add_tail(>list, _list);
> +payload_cnt++;
> +payload_version++;
> +spin_unlock_recursive(_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 

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

[Konrad Rzeszutek Wilk]

On Wed, Mar 16, 2016 at 12:12:00PM +, Julien Grall wrote:
> Hi Konrad,
> 
> On 15/03/2016 17:56, Konrad Rzeszutek Wilk wrote:
> >diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> >index 8fbc46d..dbe9ccc 100644
> >--- 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
> 
> I think it would be better to disable xSplice on ARM until we effectively
> support it.

If you would like. I made it dependent on x86.
> 
> It will avoid people asking on the mailing why xSplice doesn't work for ARM
> platform.
> 
> Regards,
> 
> -- 
> Julien Grall

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

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

[Julien Grall]

Hi Konrad,

On 15/03/2016 17:56, Konrad Rzeszutek Wilk wrote:

diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 8fbc46d..dbe9ccc 100644
--- 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


I think it would be better to disable xSplice on ARM until we 
effectively support it.


It will avoid people asking on the mailing why xSplice doesn't work for 
ARM platform.


Regards,

--
Julien Grall

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