Re: [PATCH v4 04/34] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Mar 21, 2016 at 06:45:28AM -0600, Jan Beulich wrote:
> >>> On 18.03.16 at 20:22, <konrad.wilk@oracle.com> wrote:
> >> > + * return the number of bytes requested for the operation. Or an
> >> > + * negative value if an error is encountered.
> >> > + */
> >> > +
> >> > +typedef uint64_t xen_version_op_val_t;
> >> > +DEFINE_XEN_GUEST_HANDLE(xen_version_op_val_t);
> >> > +
> >> > +typedef void xen_version_op_buf_t;
> >> > +DEFINE_XEN_GUEST_HANDLE(xen_version_op_buf_t);
> >> 
> >> Are these actually useful for anything? And for the various strings,
> > 
> > The xen_version_op_val_t is definitly used by the toolstack.
> > 
> >> wouldn't a "char" handle be more natural?
> > 
> > Heh. It was char[] before but Andrew liked it as void.
> 
> But that was because you used it for non string types too,
> wasn't it?

Yes. For the build-id which is a binary blob. And as you noticed  - also
the domain handle which can be anything.

> 
> > @@ -380,6 +388,133 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >      return -ENOSYS;
> >  }
> >  
> > +static const char *capabilities_info(unsigned int *len)
> > +{
> > +    static xen_capabilities_info_t __read_mostly cached_cap;
> > +    static unsigned int __read_mostly cached_cap_len;
> > +    static bool_t cached;
> > +
> > +    if ( unlikely(!cached) )
> > +    {
> > +        arch_get_xen_caps(&cached_cap);
> > +        cached_cap_len = strlen(cached_cap) + 1;
> > +        cached = 1;
> > +    }
> 
> I'm sorry for noticing this only now, but without any locking this is
> unsafe: x86's arch_get_xen_caps() using safe_strcat() to fill the
> buffer, simultaneous invocations would possibly produce garbled
> output to all (i.e. also subsequently started) guests. Either use a
> real lock here, or make the guard a tristate one, which gets
> transitioned e.g. from 0 to -1 by the first one coming here (doing
> the initialization), with everyone else waiting for it to become +1
> (to which the initializing party sets it once it is done).

That would indeed be bad.

What if an _init_ code called this to 'pre-cache' it?

> 
> > +DO(version_op)(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg,
> > +               unsigned int len)
> > +{
> > +    union {
> > +        xen_version_op_val_t val;
> > +        xen_feature_info_t fi;
> > +    } u = {};
> > +    unsigned int sz = 0;
> > +    const void *ptr = NULL;
> > +    int rc = xsm_version_op(XSM_OTHER, cmd);
> > +
> > +    /* We can safely return -EPERM! */
> > +    if ( rc )
> > +        return rc;
> > +
> > +    /*
> > +     * The HYPERVISOR_xen_version differs in that some return the value,
> > +     * and some copy it on back on argument. We follow the same rule for all
> > +     * sub-ops: return 0 on success, positive value of bytes returned, and
> > +     * always copy the result in arg. Yeey sanity!
> > +     */
> > +    switch ( cmd )
> > +    {
> > +    case XEN_VERSION_version:
> > +        sz = sizeof(xen_version_op_val_t);
> > +        u.val = (xen_major_version() << 16) | xen_minor_version();
> > +        break;
> > +
> > +    case XEN_VERSION_extraversion:
> > +        sz = strlen(xen_extra_version()) + 1;
> > +        ptr = xen_extra_version();
> > +        break;
> > +
> > +    case XEN_VERSION_capabilities:
> > +        ptr = capabilities_info(&sz);
> > +        break;
> > +
> > +    case XEN_VERSION_changeset:
> > +        sz = strlen(xen_changeset()) + 1;
> > +        ptr = xen_changeset();
> > +        break;
> > +
> > +    case XEN_VERSION_platform_parameters:
> > +        sz = sizeof(xen_version_op_val_t);
> > +        u.val = HYPERVISOR_VIRT_START;
> > +        break;
> > +
> > +    case XEN_VERSION_get_features:
> > +        if ( copy_from_guest(&u.fi, arg, 1) )
> 
> Afaict this is incompatible with the null handle check further down (i.e.
> you also need to check for a null handle here).

Oh my. Indeed.

> 
> > --- a/xen/include/public/arch-arm.h
> > +++ b/xen/include/public/arch-arm.h
> > @@ -128,6 +128,9 @@
> >   *    * VCPUOP_register_vcpu_info
> >   *    * VCPUOP_register_runstate_memory_area
> >   *
> > + *  HYPERVISOR_version_op
> > + *   All generic sub-operations
> > + *
> >   *
> >   * Other notes on the ARM ABI:
> 
> I don't think the extra almost blank line is warranted here.
> 
> > --- a/xen/include/public/version.h
> > +++ b/xen/include/public/version.h
> > @@ -30,7 +30,15 @@
> >  
> >  #include "xen.h"
> >  
> > -/* NB. All ops return zero on success, except XENVER_{version,pagesize} */
> > +/*
> > + * There are two hypercalls mentioned in here. The XENVER_ are for
> > + * HYPERCALL_xen_version (17), while VERSION_ are for the
> > + * HYPERCALL_version_op (41).
> > + *
> > + * The subops are very similar except that the later hypercall has a
> > + * sane interface.
> > + */
> > +
> >  
> >  /* arg == NULL; returns major:minor (16:16). */
> 
> Nor is the extra blank one here.
> 
> > @@ -87,6 +95,66 @@ typedef struct xen_feature_info xen_feature_info_t;
> >  #define XENVER_commandline 9
> >  typedef char xen_commandline_t[1024];
> >  
> > +
> > +
> > +/*
> > + * The HYPERCALL_version_op has a set of sub-ops which mirror the
> 
> And three consecutive blank lines are too much in any event. (If
> for no other reason that because that provides extremely bad
> patch context if a later change happened right next to these three
> lines.)
> 
> > +/*
> > + * arg == char.
> > + *
> > + * The toolstack fills it out for guest consumption. It is intended to hold
> > + * the UUID of the guest.
> > + */
> > +#define XEN_VERSION_guest_handle        8
> 
> So this is the place where I agree with Andrew char is not an
> appropriate type. A void or uint8 handle seems like what you
> want here.

/me nods.
> 
> > --- a/xen/include/xsm/dummy.h
> > +++ b/xen/include/xsm/dummy.h
> > @@ -751,3 +751,22 @@ static XSM_INLINE int xsm_xen_version (XSM_DEFAULT_ARG uint32_t op)
> >          return xsm_default_action(XSM_PRIV, current->domain, NULL);
> >      }
> >  }
> > +
> > +static XSM_INLINE int xsm_version_op (XSM_DEFAULT_ARG uint32_t op)
> > +{
> > +    XSM_ASSERT_ACTION(XSM_OTHER);
> > +    switch ( op )
> > +    {
> > +    case XEN_VERSION_version:
> > +    case XEN_VERSION_extraversion:
> > +    case XEN_VERSION_capabilities:
> > +    case XEN_VERSION_platform_parameters:
> > +    case XEN_VERSION_get_features:
> > +    case XEN_VERSION_pagesize:
> > +    case XEN_VERSION_guest_handle:
> > +        /* These MUST always be accessible to any guest by default. */
> > +        return xsm_default_action(XSM_HOOK, current->domain, NULL);
> > +    default:
> > +        return xsm_default_action(XSM_PRIV, current->domain, NULL);
> 
> Considering that we seem to have settled on some exceptions here
> for the change adding XSM check to the legacy version op, do you
> really think going with no exception at all here is the right approach?

> Because if we do, that'll prevent guests getting fully converted over
> to the new interface. Of course, we could also make this conversion
> specifically a non-goal, and omit e.g. XEN_VERSION_VERSION from
> this new interface.

No no. I think convesion is the right long-term goal. 

However the nice thing about this hypercall is that it can return -EPERM.

Making it always return an value for XEN_VERSION_version,
XEN_VERSION_platform_parameters, XEN_VERSION_get_features means that
there are some exceptions to this "can return -EPERM" as they will
be guaranteed an postive return value. They can ignore the -EPERM
case.

And means that guest can still take shortcuts.

I agree with you that guests need these hypercalls but at the same
time I am not sure what can be done so they don't fall flat on their
faces if they are presented with -EPERM. The Linux xenbus_init can be
modified to deal with this returning -EPERM where it makes some assumptions.

But in a likelyhood it is the bad assumption!

Andrew, what do you think?


> 
> Jan

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