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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.03.16 at 16:52, <konrad.wilk@xxxxxxxxxx> wrote:
> On Mon, Mar 21, 2016 at 06:45:28AM -0600, Jan Beulich wrote:
>> >>> On 18.03.16 at 20:22, <konrad.wilk@xxxxxxxxxx> wrote:
>> > @@ -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?

That's one of the options you have.

>> > --- 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!

I'm afraid I can't conclude what you mean to say with all of the
above.

Jan


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