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

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

On 17.11.20 13:37, Jan Beulich wrote:
> 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?
Yes, of course.

e.g. the getsize() function returning the size of a directory holding an
entry for each cpupool will need to take the cpupool lock in order to
avoid a cpupool being created or deleted in parallel.

But the cpupool create/destroy functions don't need to take the hypfs
lock.

> > --- 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?
Hmm, up to now I think such a bug would be detected rather fast.

I can add some ASSERT()s for mandatory functions not being NULL when
a node is added dynamically or during hypfs initialization for the
static nodes.

> > @@ -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?
Good question.

Up to now I think we could get away without specific alignment.

I can drop that parameter for now if you'd like that better.

> > @@ -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.
Okay.

> > @@ -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.)
I think you are right. I should add the size value as a parameter to the
read and write functions.

And no, max_size should not be dynamic.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature