Re: [PATCH 08/12] xen/hypfs: support dynamic hypfs nodes

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.10.2020 10:13, Juergen Gross wrote:
> Add a getsize() function pointer to struct hypfs_funcs for being able
> to have dynamically filled entries without the need to take the hypfs
> lock each time the contents are being generated.

But a dynamic update causing a change in size will require _some_
lock, won't it?

> --- a/xen/common/hypfs.c
> +++ b/xen/common/hypfs.c
> @@ -19,28 +19,29 @@
>  CHECK_hypfs_dirlistentry;
>  #endif
>  
> -#define DIRENTRY_NAME_OFF offsetof(struct xen_hypfs_dirlistentry, name)
> -#define DIRENTRY_SIZE(name_len) \
> -    (DIRENTRY_NAME_OFF +        \
> -     ROUNDUP((name_len) + 1, alignof(struct xen_hypfs_direntry)))
> -
>  struct hypfs_funcs hypfs_dir_funcs = {
>      .read = hypfs_read_dir,
> +    .getsize = hypfs_getsize,
> +    .findentry = hypfs_dir_findentry,
>  };
>  struct hypfs_funcs hypfs_leaf_ro_funcs = {
>      .read = hypfs_read_leaf,
> +    .getsize = hypfs_getsize,
>  };
>  struct hypfs_funcs hypfs_leaf_wr_funcs = {
>      .read = hypfs_read_leaf,
>      .write = hypfs_write_leaf,
> +    .getsize = hypfs_getsize,
>  };
>  struct hypfs_funcs hypfs_bool_wr_funcs = {
>      .read = hypfs_read_leaf,
>      .write = hypfs_write_bool,
> +    .getsize = hypfs_getsize,
>  };
>  struct hypfs_funcs hypfs_custom_wr_funcs = {
>      .read = hypfs_read_leaf,
>      .write = hypfs_write_custom,
> +    .getsize = hypfs_getsize,
>  };

With the increasing number of fields that may (deliberately or
by mistake) be NULL, should we gain some form of proactive
guarding against calls through such pointers?

> @@ -88,6 +93,23 @@ static void hypfs_unlock(void)
>      }
>  }
>  
> +void *hypfs_alloc_dyndata(unsigned long size, unsigned long align)

Will callers really need to specify (high) alignment values? IOW ...

> +{
> +    unsigned int cpu = smp_processor_id();
> +
> +    ASSERT(per_cpu(hypfs_locked, cpu) != hypfs_unlocked);
> +    ASSERT(per_cpu(hypfs_dyndata, cpu) == NULL);
> +
> +    per_cpu(hypfs_dyndata, cpu) = _xzalloc(size, align);

... is xzalloc_bytes() not suitable for use here?

> @@ -171,15 +193,34 @@ static int hypfs_get_path_user(char *buf,
>      return 0;
>  }
>  
> +struct hypfs_entry *hypfs_dir_findentry(struct hypfs_entry_dir *dir,
> +                                        const char *name,
> +                                        unsigned int name_len)
> +{
> +    struct hypfs_entry *entry;
> +
> +    list_for_each_entry ( entry, &dir->dirlist, list )
> +    {
> +        int cmp = strncmp(name, entry->name, name_len);
> +
> +        if ( cmp < 0 )
> +            return ERR_PTR(-ENOENT);
> +
> +        if ( !cmp && strlen(entry->name) == name_len )
> +            return entry;
> +    }
> +
> +    return ERR_PTR(-ENOENT);
> +}
> +
>  static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry_dir *dir,
>                                                 const char *path)
>  {
>      const char *end;
>      struct hypfs_entry *entry;
>      unsigned int name_len;
> -    bool again = true;
>  
> -    while ( again )
> +    for ( ;; )

Nit: Strictly speaking another blank is needed between the two
semicolons.

> @@ -275,22 +305,25 @@ int hypfs_read_leaf(const struct hypfs_entry *entry,
>  
>      l = container_of(entry, const struct hypfs_entry_leaf, e);
>  
> -    return copy_to_guest(uaddr, l->u.content, entry->size) ? -EFAULT: 0;
> +    return copy_to_guest(uaddr, l->u.content, entry->funcs->getsize(entry)) ?
> +                                              -EFAULT : 0;

With the intended avoiding of locking, how is this ->getsize()
guaranteed to not ...

> @@ -298,7 +331,7 @@ static int hypfs_read(const struct hypfs_entry *entry,
>          goto out;
>  
>      ret = -ENOBUFS;
> -    if ( ulen < entry->size + sizeof(e) )
> +    if ( ulen < size + sizeof(e) )
>          goto out;

... invalidate the checking done here? (A similar risk looks to
exist on the write path, albeit there we have at least the
->max_size checks, where I hope that field isn't mean to become
dynamic as well.)

Jan