Re: [Xen-devel] [PATCH v6 04/12] xen: add basic hypervisor filesystem support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.03.2020 13:00, Jürgen Groß wrote:
> On 03.03.20 17:59, Jan Beulich wrote:
>> On 26.02.2020 13:46, Juergen Gross wrote:
>>> --- /dev/null
>>> +++ b/xen/common/hypfs.c
>>> @@ -0,0 +1,349 @@
>>> +/******************************************************************************
>>> + *
>>> + * hypfs.c
>>> + *
>>> + * Simple sysfs-like file system for the hypervisor.
>>> + */
>>> +
>>> +#include <xen/err.h>
>>> +#include <xen/guest_access.h>
>>> +#include <xen/hypercall.h>
>>> +#include <xen/hypfs.h>
>>> +#include <xen/lib.h>
>>> +#include <xen/rwlock.h>
>>> +#include <public/hypfs.h>
>>> +
>>> +#ifdef CONFIG_COMPAT
>>> +#include <compat/hypfs.h>
>>> +CHECK_hypfs_direntry;
>>> +#undef CHECK_hypfs_direntry
>>> +#define CHECK_hypfs_direntry struct xen_hypfs_direntry
>>
>> I'm struggling to see why you need this #undef and #define.
> 
> Without those I get:
> 
> In file included from /home/gross/xen/unstable/xen/include/compat/xen.h:3:0,
>                   from /home/gross/xen/unstable/xen/include/xen/shared.h:6,
>                   from /home/gross/xen/unstable/xen/include/xen/sched.h:8,
>                   from /home/gross/xen/unstable/xen/include/asm/paging.h:29,
>                   from 
> /home/gross/xen/unstable/xen/include/asm/guest_access.h:1,
>                   from 
> /home/gross/xen/unstable/xen/include/xen/guest_access.h:1,
>                   from hypfs.c:9:
> /home/gross/xen/unstable/xen/include/xen/compat.h:134:32: error: 
> redefinition of ‘__checkFstruct_hypfs_direntry__flags’
>   #define CHECK_NAME_(k, n, tag) __check ## tag ## k ## _ ## n
>                                  ^
> /home/gross/xen/unstable/xen/include/xen/compat.h:166:34: note: in 
> definition of macro ‘CHECK_FIELD_COMMON_’
>   static inline int __maybe_unused name(k xen_ ## n *x, k compat_ ## n *c) \
>                                    ^~~~
> /home/gross/xen/unstable/xen/include/xen/compat.h:176:28: note: in 
> expansion of macro ‘CHECK_NAME_’
>       CHECK_FIELD_COMMON_(k, CHECK_NAME_(k, n ## __ ## f, F), n, f)
>                              ^~~~~~~~~~~
> /home/gross/xen/unstable/xen/include/compat/xlat.h:775:5: note: in 
> expansion of macro ‘CHECK_FIELD_’
>       CHECK_FIELD_(struct, hypfs_direntry, flags); \
>       ^~~~~~~~~~~~
> /home/gross/xen/unstable/xen/include/compat/xlat.h:782:5: note: in 
> expansion of macro ‘CHECK_hypfs_direntry’
>       CHECK_hypfs_direntry; \
>       ^~~~~~~~~~~~~~~~~~~~
> hypfs.c:19:1: note: in expansion of macro ‘CHECK_hypfs_dirlistentry’
>   CHECK_hypfs_dirlistentry;
>   ^~~~~~~~~~~~~~~~~~~~~~~~
> /home/gross/xen/unstable/xen/include/xen/compat.h:134:32: note: previous 
> definition of ‘__checkFstruct_hypfs_direntry__flags’ was here
>   #define CHECK_NAME_(k, n, tag) __check ## tag ## k ## _ ## n
>                                  ^
> /home/gross/xen/unstable/xen/include/xen/compat.h:166:34: note: in 
> definition of macro ‘CHECK_FIELD_COMMON_’
>   static inline int __maybe_unused name(k xen_ ## n *x, k compat_ ## n *c) \
>                                    ^~~~
> /home/gross/xen/unstable/xen/include/xen/compat.h:176:28: note: in 
> expansion of macro ‘CHECK_NAME_’
>       CHECK_FIELD_COMMON_(k, CHECK_NAME_(k, n ## __ ## f, F), n, f)
>                              ^~~~~~~~~~~
> /home/gross/xen/unstable/xen/include/compat/xlat.h:775:5: note: in 
> expansion of macro ‘CHECK_FIELD_’
>       CHECK_FIELD_(struct, hypfs_direntry, flags); \
>       ^~~~~~~~~~~~
> hypfs.c:18:1: note: in expansion of macro ‘CHECK_hypfs_direntry’
>   CHECK_hypfs_direntry;

Which suggests to me that the explicit CHECK_hypfs_direntry invocation
is unneeded, as it's getting verified as part of the invocation of
CHECK_hypfs_dirlistentry.

>>> +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;
>>> +
>>> +    if ( leaf->e.type != XEN_HYPFS_TYPE_STRING &&
>>> +         leaf->e.type != XEN_HYPFS_TYPE_BLOB && ulen != leaf->e.size )
>>> +        return -EDOM;
>>
>> Why the exception of string and blob? My concern about the
>> meaning of a partially written entry (without its size having
>> changed) remains.
> 
> It is perfectly valid to write a shorter string into a character
> array. I could drop the blob here, but in the end I think allowing
> for a blob to change the size should be fine.

But shouldn't this then also adjust the recorded size?

>>> +    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 && !memchr(buf, 0, ulen) )
>>
>> This should also use the != buf + ulen - 1 form imo.
> 
> I'm fine to change that, but should the hypervisor really refuse to
> accept a larger buffer?

To avoid ambiguity I'd prefer if the requirement was that the
caller specify the length of the string (plus the nul char)
rather than the size of any buffer it might be using.

Jan

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