| Message ID | 20201026091316.25680-9-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: support per-cpupool scheduling granularity | expand |
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
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
On 17.11.2020 15:29, Jürgen Groß wrote: > On 17.11.20 13:37, Jan Beulich wrote: >> On 26.10.2020 10:13, Juergen Gross wrote: >>> --- 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. Not sure: Are there any unavoidable uses of all affected code paths? > 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. I'm not sure ASSERT()s alone are enough. I'd definitely prefer something which at least avoids the obvious x86 PV privilege escalation attack in case a call through NULL has gone unnoticed earlier on. E.g. rather than storing NULL in unused entries, store a non-canonical pointer so that the effect will "just" be a DoS. >>> @@ -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. I think control over alignment should be limited to those special cases really needing it. >>> @@ -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. Except that a function like hypfs_read_leaf() shouldn't really require its caller to pass in the size of the leaf's payload. Jan
On 17.11.20 15:40, Jan Beulich wrote: > On 17.11.2020 15:29, Jürgen Groß wrote: >> On 17.11.20 13:37, Jan Beulich wrote: >>> On 26.10.2020 10:13, Juergen Gross wrote: >>>> --- 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. > > Not sure: Are there any unavoidable uses of all affected code > paths? Assuming that any new node implementation is tested at least once, yes. But in general you are right, of course. > >> 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. > > I'm not sure ASSERT()s alone are enough. I'd definitely prefer > something which at least avoids the obvious x86 PV privilege > escalation attack in case a call through NULL has gone > unnoticed earlier on. E.g. rather than storing NULL in unused > entries, store a non-canonical pointer so that the effect will > "just" be a DoS. Hmm, either this, or a dummy function doing no harm? > >>>> @@ -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. > > I think control over alignment should be limited to those > special cases really needing it. Okay, I'll drop the align parameter then. > >>>> @@ -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. > > Except that a function like hypfs_read_leaf() shouldn't really > require its caller to pass in the size of the leaf's payload. Its not the leaf's payload size, but the size the user buffer has been tested against. Juergen
diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c index 97260bd4a3..4c226a06b4 100644 --- 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, }; static DEFINE_RWLOCK(hypfs_lock); @@ -50,6 +51,7 @@ enum hypfs_lock_state { hypfs_write_locked }; static DEFINE_PER_CPU(enum hypfs_lock_state, hypfs_locked); +static DEFINE_PER_CPU(void *, hypfs_dyndata); HYPFS_DIR_INIT(hypfs_root, ""); @@ -71,9 +73,12 @@ static void hypfs_write_lock(void) static void hypfs_unlock(void) { - enum hypfs_lock_state locked = this_cpu(hypfs_locked); + unsigned int cpu = smp_processor_id(); + enum hypfs_lock_state locked = per_cpu(hypfs_locked, cpu); + + XFREE(per_cpu(hypfs_dyndata, cpu)); - this_cpu(hypfs_locked) = hypfs_unlocked; + per_cpu(hypfs_locked, cpu) = hypfs_unlocked; switch ( locked ) { @@ -88,6 +93,23 @@ static void hypfs_unlock(void) } } +void *hypfs_alloc_dyndata(unsigned long size, unsigned long align) +{ + 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); + + return per_cpu(hypfs_dyndata, cpu); +} + +void *hypfs_get_dyndata(void) +{ + return this_cpu(hypfs_dyndata); +} + static int add_entry(struct hypfs_entry_dir *parent, struct hypfs_entry *new) { int ret = -ENOENT; @@ -122,7 +144,7 @@ static int add_entry(struct hypfs_entry_dir *parent, struct hypfs_entry *new) { unsigned int sz = strlen(new->name); - parent->e.size += DIRENTRY_SIZE(sz); + parent->e.size += HYPFS_DIRENTRY_SIZE(sz); } hypfs_unlock(); @@ -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 ( ;; ) { if ( dir->e.type != XEN_HYPFS_TYPE_DIR ) return ERR_PTR(-ENOENT); @@ -192,28 +233,12 @@ static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry_dir *dir, end = strchr(path, '\0'); name_len = end - path; - again = false; + entry = dir->e.funcs->findentry(dir, path, name_len); + if ( IS_ERR(entry) || !*end ) + return entry; - list_for_each_entry ( entry, &dir->dirlist, list ) - { - int cmp = strncmp(path, entry->name, name_len); - struct hypfs_entry_dir *d = container_of(entry, - struct hypfs_entry_dir, e); - - if ( cmp < 0 ) - return ERR_PTR(-ENOENT); - if ( !cmp && strlen(entry->name) == name_len ) - { - if ( !*end ) - return entry; - - again = true; - dir = d; - path = end + 1; - - break; - } - } + path = end + 1; + dir = container_of(entry, struct hypfs_entry_dir, e); } return ERR_PTR(-ENOENT); @@ -227,12 +252,17 @@ static struct hypfs_entry *hypfs_get_entry(const char *path) return hypfs_get_entry_rel(&hypfs_root, path + 1); } +unsigned int hypfs_getsize(const struct hypfs_entry *entry) +{ + return entry->size; +} + int hypfs_read_dir(const struct hypfs_entry *entry, XEN_GUEST_HANDLE_PARAM(void) uaddr) { const struct hypfs_entry_dir *d; const struct hypfs_entry *e; - unsigned int size = entry->size; + unsigned int size = entry->funcs->getsize(entry); ASSERT(this_cpu(hypfs_locked) != hypfs_unlocked); @@ -242,18 +272,18 @@ int hypfs_read_dir(const struct hypfs_entry *entry, { struct xen_hypfs_dirlistentry direntry; unsigned int e_namelen = strlen(e->name); - unsigned int e_len = DIRENTRY_SIZE(e_namelen); + unsigned int e_len = HYPFS_DIRENTRY_SIZE(e_namelen); direntry.e.pad = 0; direntry.e.type = e->type; direntry.e.encoding = e->encoding; - direntry.e.content_len = e->size; + direntry.e.content_len = e->funcs->getsize(e); direntry.e.max_write_len = e->max_size; direntry.off_next = list_is_last(&e->list, &d->dirlist) ? 0 : e_len; if ( copy_to_guest(uaddr, &direntry, 1) ) return -EFAULT; - if ( copy_to_guest_offset(uaddr, DIRENTRY_NAME_OFF, + if ( copy_to_guest_offset(uaddr, HYPFS_DIRENTRY_NAME_OFF, e->name, e_namelen + 1) ) return -EFAULT; @@ -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; } static int hypfs_read(const struct hypfs_entry *entry, XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen) { struct xen_hypfs_direntry e; + unsigned int size; long ret = -EINVAL; if ( ulen < sizeof(e) ) goto out; + size = entry->funcs->getsize(entry); e.pad = 0; e.type = entry->type; e.encoding = entry->encoding; - e.content_len = entry->size; + e.content_len = size; e.max_write_len = entry->max_size; ret = -EFAULT; @@ -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; guest_handle_add_offset(uaddr, sizeof(e)); @@ -314,14 +347,15 @@ int hypfs_write_leaf(struct hypfs_entry_leaf *leaf, { char *buf; int ret; + struct hypfs_entry *e = &leaf->e; ASSERT(this_cpu(hypfs_locked) == hypfs_write_locked); - if ( ulen > leaf->e.max_size ) + if ( ulen > e->max_size ) return -ENOSPC; - if ( leaf->e.type != XEN_HYPFS_TYPE_STRING && - leaf->e.type != XEN_HYPFS_TYPE_BLOB && ulen != leaf->e.size ) + if ( e->type != XEN_HYPFS_TYPE_STRING && + e->type != XEN_HYPFS_TYPE_BLOB && ulen != e->funcs->getsize(e) ) return -EDOM; buf = xmalloc_array(char, ulen); @@ -333,14 +367,14 @@ int hypfs_write_leaf(struct hypfs_entry_leaf *leaf, goto out; ret = -EINVAL; - if ( leaf->e.type == XEN_HYPFS_TYPE_STRING && - leaf->e.encoding == XEN_HYPFS_ENC_PLAIN && + if ( e->type == XEN_HYPFS_TYPE_STRING && + e->encoding == XEN_HYPFS_ENC_PLAIN && memchr(buf, 0, ulen) != (buf + ulen - 1) ) goto out; ret = 0; memcpy(leaf->u.write_ptr, buf, ulen); - leaf->e.size = ulen; + e->size = ulen; out: xfree(buf); @@ -354,7 +388,7 @@ int hypfs_write_bool(struct hypfs_entry_leaf *leaf, ASSERT(this_cpu(hypfs_locked) == hypfs_write_locked); ASSERT(leaf->e.type == XEN_HYPFS_TYPE_BOOL && - leaf->e.size == sizeof(bool) && + leaf->e.funcs->getsize(&leaf->e) == sizeof(bool) && leaf->e.max_size == sizeof(bool) ); if ( ulen != leaf->e.max_size ) diff --git a/xen/include/xen/hypfs.h b/xen/include/xen/hypfs.h index 77916ebb58..c8999b5381 100644 --- a/xen/include/xen/hypfs.h +++ b/xen/include/xen/hypfs.h @@ -2,11 +2,13 @@ #define __XEN_HYPFS_H__ #ifdef CONFIG_HYPFS +#include <xen/lib.h> #include <xen/list.h> #include <xen/string.h> #include <public/hypfs.h> struct hypfs_entry_leaf; +struct hypfs_entry_dir; struct hypfs_entry; struct hypfs_funcs { @@ -14,6 +16,9 @@ struct hypfs_funcs { XEN_GUEST_HANDLE_PARAM(void) uaddr); int (*write)(struct hypfs_entry_leaf *leaf, XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned int ulen); + unsigned int (*getsize)(const struct hypfs_entry *entry); + struct hypfs_entry *(*findentry)(struct hypfs_entry_dir *dir, + const char *name, unsigned int name_len); }; extern struct hypfs_funcs hypfs_dir_funcs; @@ -45,7 +50,12 @@ struct hypfs_entry_dir { struct list_head dirlist; }; -#define HYPFS_DIR_INIT(var, nam) \ +#define HYPFS_DIRENTRY_NAME_OFF offsetof(struct xen_hypfs_dirlistentry, name) +#define HYPFS_DIRENTRY_SIZE(name_len) \ + (HYPFS_DIRENTRY_NAME_OFF + \ + ROUNDUP((name_len) + 1, alignof(struct xen_hypfs_direntry))) + +#define HYPFS_VARDIR_INIT(var, nam, fn) \ struct hypfs_entry_dir __read_mostly var = { \ .e.type = XEN_HYPFS_TYPE_DIR, \ .e.encoding = XEN_HYPFS_ENC_PLAIN, \ @@ -53,22 +63,25 @@ struct hypfs_entry_dir { .e.size = 0, \ .e.max_size = 0, \ .e.list = LIST_HEAD_INIT(var.e.list), \ - .e.funcs = &hypfs_dir_funcs, \ + .e.funcs = (fn), \ .dirlist = LIST_HEAD_INIT(var.dirlist), \ } -#define HYPFS_VARSIZE_INIT(var, typ, nam, msz) \ - struct hypfs_entry_leaf __read_mostly var = { \ - .e.type = (typ), \ - .e.encoding = XEN_HYPFS_ENC_PLAIN, \ - .e.name = (nam), \ - .e.max_size = (msz), \ - .e.funcs = &hypfs_leaf_ro_funcs, \ +#define HYPFS_DIR_INIT(var, nam) \ + HYPFS_VARDIR_INIT(var, nam, &hypfs_dir_funcs) + +#define HYPFS_VARSIZE_INIT(var, typ, nam, msz, fn) \ + struct hypfs_entry_leaf __read_mostly var = { \ + .e.type = (typ), \ + .e.encoding = XEN_HYPFS_ENC_PLAIN, \ + .e.name = (nam), \ + .e.max_size = (msz), \ + .e.funcs = (fn), \ } /* Content and size need to be set via hypfs_string_set_reference(). */ #define HYPFS_STRING_INIT(var, nam) \ - HYPFS_VARSIZE_INIT(var, XEN_HYPFS_TYPE_STRING, nam, 0) + HYPFS_VARSIZE_INIT(var, XEN_HYPFS_TYPE_STRING, nam, 0, &hypfs_leaf_ro_funcs) /* * Set content and size of a XEN_HYPFS_TYPE_STRING node. The node will point @@ -131,6 +144,12 @@ int hypfs_write_bool(struct hypfs_entry_leaf *leaf, XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned int ulen); int hypfs_write_custom(struct hypfs_entry_leaf *leaf, XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned int ulen); +unsigned int hypfs_getsize(const struct hypfs_entry *entry); +struct hypfs_entry *hypfs_dir_findentry(struct hypfs_entry_dir *dir, + const char *name, + unsigned int name_len); +void *hypfs_alloc_dyndata(unsigned long size, unsigned long align); +void *hypfs_get_dyndata(void); #endif #endif /* __XEN_HYPFS_H__ */
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. For directories add a findentry callback to the vector and modify hypfs_get_entry_rel() to use it. Add a HYPFS_VARDIR_INIT() macro for intializing such a directory statically, taking a struct hypfs_funcs pointer as parameter additional to those of HYPFS_DIR_INIT(). Modify HYPFS_VARSIZE_INIT() to take the function vector pointer as an additional parameter as this will be needed for dynamical entries. Move DIRENTRY_SIZE() macro to hypfs.h as it will be needed by the read function of a directory with dynamically generated entries. For being able to let the generic hypfs coding continue to work on normal struct hypfs_entry entities even for dynamical nodes add some infrastructure for allocating a working area for the current hypfs request in order to store needed information for traversing the tree. This area is anchored in a percpu pointer and can be retrieved by any level of the dynamic entries. It will be freed automatically when dropping the hypfs lock. Signed-off-by: Juergen Gross <jgross@suse.com> --- xen/common/hypfs.c | 124 +++++++++++++++++++++++++--------------- xen/include/xen/hypfs.h | 39 +++++++++---- 2 files changed, 108 insertions(+), 55 deletions(-)