Re: [Xen-devel] [PATCH v5 3/8] xen: add basic hypervisor filesystem support

[Jürgen Groß <jgross@xxxxxxxx>]

On 19.02.20 16:49, Jan Beulich wrote:
> On 19.02.2020 09:11, Juergen Gross wrote:
> > +static int hypfs_get_path_user(char *buf,
> > +                               XEN_GUEST_HANDLE_PARAM(const_char) uaddr,
> > +                               unsigned long ulen)
> > +{
> > +    if ( ulen > XEN_HYPFS_MAX_PATHLEN )
> > +        return -EINVAL;
> > +
> > +    if ( copy_from_guest(buf, uaddr, ulen) )
> > +        return -EFAULT;
> > +
> > +    if ( buf[ulen - 1] )
> > +        return -EINVAL;
> I (still, but I may not have said so before) wonder whether
> memchr(buf, 0, ulen) != buf + ulen - 1 wouldn't be better here.
I like the idea.

> > +int hypfs_write_leaf(struct hypfs_entry_leaf *leaf,
> > +                     XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
> > +{
> > +    char *buf;
> > +    int ret;
> > +
> > +    if ( ulen > leaf->e.size )
> > +        return -ENOSPC;
> Okay, this makes sure you don't overrun the internal buffer.
> What about the opposite mismatch (ulen < leaf->e.size)? The
> result, except perhaps for (nul-terminated) strings, is not
> going to be very useful, at the very least.
Yes, will tighten the test.

> > +    buf = xmalloc_array(char, ulen);
> > +    if ( !buf )
> > +        return -ENOMEM;
> > +
> > +    ret = -EFAULT;
> > +    if ( copy_from_guest(buf, uaddr, ulen) )
> > +        goto out;
> > +
> > +    ret = -EINVAL;
> > +    if ( leaf->e.type == XEN_HYPFS_TYPE_STRING && buf[ulen] )
> buf[ulen - 1] I guess? Or, as above, memchr() again?
I'll go the memchr() way.

> > +int hypfs_write_bool(struct hypfs_entry_leaf *leaf,
> > +                     XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
> > +{
> > +    union {
> > +        char buf[8];
> > +        uint8_t u8;
> > +        uint16_t u16;
> > +        uint32_t u32;
> > +        uint64_t u64;
> > +    } u;
> > +
> > +    ASSERT(leaf->e.type == XEN_HYPFS_TYPE_UINT && leaf->e.size <= 8);
> > +
> > +    if ( ulen != leaf->e.size )
> > +        return -EDOM;
> > +
> > +    if ( copy_from_guest(u.buf, uaddr, ulen) )
> > +        return -EFAULT;
> > +
> > +    switch ( leaf->e.size )
> > +    {
> > +    case 1:
> > +        *(uint8_t *)leaf->write_ptr = !!u.u8;
> > +        break;
> > +    case 2:
> > +        *(uint16_t *)leaf->write_ptr = !!u.u16;
> > +        break;
> > +    case 4:
> > +        *(uint32_t *)leaf->write_ptr = !!u.u32;
> > +        break;
> > +    case 8:
> > +        *(uint64_t *)leaf->write_ptr = !!u.u64;
> > +        break;
> > +    }
> Looking at this again, is there really a need for uint64_t support
> here? I.e. can't you cap at unsigned long (or even unsigned int),
> and perhaps additionally avoid use of fixed width types here
> altogether (some trickery may be needed for 32-bit's
> sizeof(long) == sizeof(int))?
The reason for different lengths for bool is that boolean_param() is
falling back to the general integer param handling with all the natural
integer sizes.

I think I'll modify the macros in param.h to accept only sizeof(bool)
variables for boolean parameters and then I can switch to this fixed
size here.

> > --- /dev/null
> > +++ b/xen/include/public/hypfs.h
> > @@ -0,0 +1,127 @@
> > +/******************************************************************************
> > + * Xen Hypervisor Filesystem
> > + *
> > + * Copyright (c) 2019, SUSE Software Solutions Germany GmbH
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a copy
> > + * of this software and associated documentation files (the "Software"), to
> > + * deal in the Software without restriction, including without limitation the
> > + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
> > + * sell copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> > + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > + * DEALINGS IN THE SOFTWARE.
> > + *
> > + */
> > +
> > +#ifndef __XEN_PUBLIC_HYPFS_H__
> > +#define __XEN_PUBLIC_HYPFS_H__
> > +
> > +#include "xen.h"
> > +
> > +/*
> > + * Definitions for the __HYPERVISOR_hypfs_op hypercall.
> > + */
> > +
> > +/* Highest version number of the hypfs interface currently defined. */
> > +#define XEN_HYPFS_VERSION      1
> > +
> > +/* Maximum length of a path in the filesystem. */
> > +#define XEN_HYPFS_MAX_PATHLEN 1024
> I think it would be nice if the blank padding here matched that
> of the other #define-s up and down from here.
Okay.

> > +/*
> > + * XEN_HYPFS_OP_get_version
> > + *
> > + * Read highest interface version supported by the hypervisor.
> > + *
> > + * Possible return values:
> > + * >0: highest supported interface version
> > + * <0: negative Xen errno value
> > + */
> > +#define XEN_HYPFS_OP_get_version     0
> > +
> > +/*
> > + * XEN_HYPFS_OP_read
> > + *
> > + * Read a filesystem entry.
> > + *
> > + * Returns the direntry and contents of an entry in the buffer supplied by the
> > + * caller (struct xen_hypfs_direntry with the contents following directly
> > + * after it).
> > + * The data buffer must be at least the size of the direntry returned in order
> > + * to have success. If the data buffer was not large enough for all the data
> Looks like the "to have success" is stale now?
Yes.

> > + * -ENOBUFS and no entry data is returned, but the direntry will contain the
> > + * needed size for the returned data.
> > + * The format of the contents is according to its entry type and encoding.
> > + * The contents of a directory are multiple struct xen_hypfs_dirlistentry
> > + * items.
> > + *
> > + * arg1: XEN_GUEST_HANDLE(path name)
> > + * arg2: length of path name (including trailing zero byte)
> > + * arg3: XEN_GUEST_HANDLE(data buffer written by hypervisor)
> > + * arg4: data buffer size
> > + *
> > + * Possible return values:
> > + * 0: success (at least the direntry was returned)
> As is this?
Yes.

> > +static inline void hypfs_string_set(struct hypfs_entry_leaf *leaf,
> > +                                    const char *str)
> > +{
> > +    leaf->content = str;
> > +    leaf->e.size = strlen(str) + 1;
> > +}
> This looks at least risky to me, as the function name does in
> no way indicate that no copy of the string will be made. Hence
> its use with e.g. .init.rodata contents or a stack variable
> will not produce the intended result.
Okay, what about naming it hypfs_string_set_reference() ?

> > +#define HYPFS_UINT_INIT(var, nam, contvar)        \
> > +    struct hypfs_entry_leaf __read_mostly var = { \
> > +        .e.type = XEN_HYPFS_TYPE_UINT,            \
> > +        .e.encoding = XEN_HYPFS_ENC_PLAIN,        \
> > +        .e.name = nam,                            \
> > +        .e.size = sizeof(contvar),                \
> > +        .e.read = hypfs_read_leaf,                \
> > +        .content = &contvar,                      \
> > +    }
> > +
> > +#define HYPFS_INT_INIT(var, nam, contvar)         \
> > +    struct hypfs_entry_leaf __read_mostly var = { \
> > +        .e.type = XEN_HYPFS_TYPE_INT,             \
> > +        .e.encoding = XEN_HYPFS_ENC_PLAIN,        \
> > +        .e.name = nam,                            \
> > +        .e.size = sizeof(contvar),                \
> > +        .e.read = hypfs_read_leaf,                \
> > +        .content = &contvar,                      \
> > +    }
> > +
> > +#define HYPFS_BOOL_INIT(var, nam, contvar)        \
> > +    struct hypfs_entry_leaf __read_mostly var = { \
> > +        .e.type = XEN_HYPFS_TYPE_BOOL,            \
> > +        .e.encoding = XEN_HYPFS_ENC_PLAIN,        \
> > +        .e.name = nam,                            \
> > +        .e.size = sizeof(contvar),                \
> > +        .e.read = hypfs_read_leaf,                \
> > +        .content = &contvar,                      \
> > +    }
> Quite a lot of redundancy for just a single line of difference
> between the instance. Perhaps have another helper macro?
Fine with me.

> > --- a/xen/include/xlat.lst
> > +++ b/xen/include/xlat.lst
> > @@ -86,6 +86,8 @@
> >   ?     vcpu_hvm_context                hvm/hvm_vcpu.h
> >   ?     vcpu_hvm_x86_32                 hvm/hvm_vcpu.h
> >   ?     vcpu_hvm_x86_64                 hvm/hvm_vcpu.h
> > +?      xen_hypfs_direntry              hypfs.h
> > +?      xen_hypfs_dirlistentry          hypfs.h
> Where are the checking macros used that these produce?
Ah, sorry. Will add them.

> > --- a/xen/xsm/flask/policy/access_vectors
> > +++ b/xen/xsm/flask/policy/access_vectors
> > @@ -67,6 +67,8 @@ class xen
> >       lockprof
> >   # XEN_SYSCTL_cpupool_op
> >       cpupool_op
> > +# hypfs hypercall
> > +    hypfs_op
> >   # XEN_SYSCTL_scheduler_op with XEN_DOMCTL_SCHEDOP_getinfo, 
> > XEN_SYSCTL_sched_id, XEN_DOMCTL_SCHEDOP_getvcpuinfo
> >       getscheduler
> >   # XEN_SYSCTL_scheduler_op with XEN_DOMCTL_SCHEDOP_putinfo, 
> > XEN_DOMCTL_SCHEDOP_putvcpuinfo
> I may not know enough about XSM to see why you can get away without
> also modifying flask/hooks.c.
Hmm, strange, I was sure to have added it.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel