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

[Jan Beulich <jbeulich@suse.com>]

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.

> +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.

> +    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.

Jan

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