diff mbox series

[v2,09/17] xen/hypfs: move per-node function pointers into a dedicated struct

Message ID 20201201082128.15239-10-jgross@suse.com (mailing list archive)
State Superseded
Headers show
Series xen: support per-cpupool scheduling granularity | expand

Commit Message

Jürgen Groß Dec. 1, 2020, 8:21 a.m. UTC
Move the function pointers currently stored in each hypfs node into a
dedicated structure in order to save some space for each node. This
will save even more space with additional callbacks added in future.

Provide some standard function vectors.

Instead of testing the write pointer to be not NULL provide a dummy
function just returning -EACCESS. ASSERT() all vector entries being
populated when adding a node. This avoids any potential problem (e.g.
pv domain privilege escalations) in case of calling a non populated
vector entry.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- make function vector const (Jan Beulich)
- don't allow any NULL entry (Jan Beulich)
- add callback comment
---
 xen/common/hypfs.c      | 41 ++++++++++++++++++++----
 xen/include/xen/hypfs.h | 71 ++++++++++++++++++++++++++++-------------
 xen/include/xen/param.h | 15 +++------
 3 files changed, 88 insertions(+), 39 deletions(-)

Comments

Jan Beulich Dec. 2, 2020, 3:36 p.m. UTC | #1
On 01.12.2020 09:21, Juergen Gross wrote:
> @@ -297,6 +321,7 @@ int hypfs_write_leaf(struct hypfs_entry_leaf *leaf,
>      int ret;
>  
>      ASSERT(this_cpu(hypfs_locked) == hypfs_write_locked);
> +    ASSERT(leaf->e.max_size);
>  
>      if ( ulen > leaf->e.max_size )
>          return -ENOSPC;
> @@ -357,6 +382,7 @@ int hypfs_write_custom(struct hypfs_entry_leaf *leaf,
>      int ret;
>  
>      ASSERT(this_cpu(hypfs_locked) == hypfs_write_locked);
> +    ASSERT(leaf->e.max_size);
>  
>      /* Avoid oversized buffer allocation. */
>      if ( ulen > MAX_PARAM_SIZE )

At the first glance both of these hunks look as if they
wouldn't belong here, but I guess the ASSERT()s are getting
lifted here from hypfs_write(). (The first looks even somewhat
redundant with the immediately following if().) If this
understanding of mine is correct,
Reviewed-by: Jan Beulich <jbeulich@suse.com>

> @@ -382,19 +408,20 @@ int hypfs_write_custom(struct hypfs_entry_leaf *leaf,
>      return ret;
>  }
>  
> +int hypfs_write_deny(struct hypfs_entry_leaf *leaf,
> +                     XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned int ulen)
> +{
> +    return -EACCES;
> +}
> +
>  static int hypfs_write(struct hypfs_entry *entry,
>                         XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)

As a tangent, is there a reason these write functions don't take
handles of "const void"? (I realize this likely is nothing that
wants addressing right here.)

Jan
Jürgen Groß Dec. 2, 2020, 3:41 p.m. UTC | #2
On 02.12.20 16:36, Jan Beulich wrote:
> On 01.12.2020 09:21, Juergen Gross wrote:
>> @@ -297,6 +321,7 @@ int hypfs_write_leaf(struct hypfs_entry_leaf *leaf,
>>       int ret;
>>   
>>       ASSERT(this_cpu(hypfs_locked) == hypfs_write_locked);
>> +    ASSERT(leaf->e.max_size);
>>   
>>       if ( ulen > leaf->e.max_size )
>>           return -ENOSPC;
>> @@ -357,6 +382,7 @@ int hypfs_write_custom(struct hypfs_entry_leaf *leaf,
>>       int ret;
>>   
>>       ASSERT(this_cpu(hypfs_locked) == hypfs_write_locked);
>> +    ASSERT(leaf->e.max_size);
>>   
>>       /* Avoid oversized buffer allocation. */
>>       if ( ulen > MAX_PARAM_SIZE )
> 
> At the first glance both of these hunks look as if they
> wouldn't belong here, but I guess the ASSERT()s are getting
> lifted here from hypfs_write(). (The first looks even somewhat
> redundant with the immediately following if().) If this
> understanding of mine is correct,

It is.

> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

> 
>> @@ -382,19 +408,20 @@ int hypfs_write_custom(struct hypfs_entry_leaf *leaf,
>>       return ret;
>>   }
>>   
>> +int hypfs_write_deny(struct hypfs_entry_leaf *leaf,
>> +                     XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned int ulen)
>> +{
>> +    return -EACCES;
>> +}
>> +
>>   static int hypfs_write(struct hypfs_entry *entry,
>>                          XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
> 
> As a tangent, is there a reason these write functions don't take
> handles of "const void"? (I realize this likely is nothing that
> wants addressing right here.)

No, not really.

I'll change that.


Juergen
Jürgen Groß Dec. 3, 2020, 8:47 a.m. UTC | #3
On 02.12.20 16:36, Jan Beulich wrote:
> On 01.12.2020 09:21, Juergen Gross wrote:
>>   static int hypfs_write(struct hypfs_entry *entry,
>>                          XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
> 
> As a tangent, is there a reason these write functions don't take
> handles of "const void"? (I realize this likely is nothing that
> wants addressing right here.)

Uh, this is harder than I thought.

guest_handle_cast() doesn't handle const guest handle types currently:

hypfs.c:447:58: error: unknown type name ‘const_void’; did you mean ‘const’?
          ret = hypfs_write(entry, guest_handle_cast(arg3, const_void), 
arg4);
                                                           ^
/home/gross/xen/unstable/xen/include/xen/guest_access.h:26:5: note: in 
definition of macro ‘guest_handle_cast’
      type *_x = (hnd).p;                         \
      ^~~~

Currently my ideas would be to either:

- add a new macro for constifying a guest handle (type -> const_type)
- add a new macro for casting a guest handle to a const_type
- add typedefs for the const_* types (typedef const x const_x)
- open code the cast

Or am I missing an existing variant?


Juergen
Jan Beulich Dec. 3, 2020, 9:12 a.m. UTC | #4
On 03.12.2020 09:47, Jürgen Groß wrote:
> On 02.12.20 16:36, Jan Beulich wrote:
>> On 01.12.2020 09:21, Juergen Gross wrote:
>>>   static int hypfs_write(struct hypfs_entry *entry,
>>>                          XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
>>
>> As a tangent, is there a reason these write functions don't take
>> handles of "const void"? (I realize this likely is nothing that
>> wants addressing right here.)
> 
> Uh, this is harder than I thought.
> 
> guest_handle_cast() doesn't handle const guest handle types currently:
> 
> hypfs.c:447:58: error: unknown type name ‘const_void’; did you mean ‘const’?
>           ret = hypfs_write(entry, guest_handle_cast(arg3, const_void), 
> arg4);
>                                                            ^
> /home/gross/xen/unstable/xen/include/xen/guest_access.h:26:5: note: in 
> definition of macro ‘guest_handle_cast’
>       type *_x = (hnd).p;                         \
>       ^~~~
> 
> Currently my ideas would be to either:
> 
> - add a new macro for constifying a guest handle (type -> const_type)
> - add a new macro for casting a guest handle to a const_type
> - add typedefs for the const_* types (typedef const x const_x)
> - open code the cast
> 
> Or am I missing an existing variant?

I don't think you are. Both of your first two suggestions look good
to me - ultimately we may want to have both anyway, eventually. For
its (presumed) type safety I may have a slight preference for
option 1, albeit afaics guest_handle_cast() doesn't allow
conversion between arbitrary types either (only to/from void).

It's quite unfortunate that this requires an explicit cast in the
first place, but what do you do.

Jan
Jürgen Groß Dec. 3, 2020, 9:51 a.m. UTC | #5
On 03.12.20 10:12, Jan Beulich wrote:
> On 03.12.2020 09:47, Jürgen Groß wrote:
>> On 02.12.20 16:36, Jan Beulich wrote:
>>> On 01.12.2020 09:21, Juergen Gross wrote:
>>>>    static int hypfs_write(struct hypfs_entry *entry,
>>>>                           XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
>>>
>>> As a tangent, is there a reason these write functions don't take
>>> handles of "const void"? (I realize this likely is nothing that
>>> wants addressing right here.)
>>
>> Uh, this is harder than I thought.
>>
>> guest_handle_cast() doesn't handle const guest handle types currently:
>>
>> hypfs.c:447:58: error: unknown type name ‘const_void’; did you mean ‘const’?
>>            ret = hypfs_write(entry, guest_handle_cast(arg3, const_void),
>> arg4);
>>                                                             ^
>> /home/gross/xen/unstable/xen/include/xen/guest_access.h:26:5: note: in
>> definition of macro ‘guest_handle_cast’
>>        type *_x = (hnd).p;                         \
>>        ^~~~
>>
>> Currently my ideas would be to either:
>>
>> - add a new macro for constifying a guest handle (type -> const_type)
>> - add a new macro for casting a guest handle to a const_type
>> - add typedefs for the const_* types (typedef const x const_x)
>> - open code the cast
>>
>> Or am I missing an existing variant?
> 
> I don't think you are. Both of your first two suggestions look good
> to me - ultimately we may want to have both anyway, eventually. For
> its (presumed) type safety I may have a slight preference for
> option 1, albeit afaics guest_handle_cast() doesn't allow
> conversion between arbitrary types either (only to/from void).
> 
> It's quite unfortunate that this requires an explicit cast in the
> first place, but what do you do.

Right.

I'm going with variant 2, as variant 1 is not really easy to achieve
without specifying the basic type as a macro parameter - which will
basically be variant 2.


Juergen
diff mbox series

Patch

diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c
index 8e932b5cf9..7befd144ba 100644
--- a/xen/common/hypfs.c
+++ b/xen/common/hypfs.c
@@ -24,6 +24,27 @@  CHECK_hypfs_dirlistentry;
     (DIRENTRY_NAME_OFF +        \
      ROUNDUP((name_len) + 1, alignof(struct xen_hypfs_direntry)))
 
+const struct hypfs_funcs hypfs_dir_funcs = {
+    .read = hypfs_read_dir,
+    .write = hypfs_write_deny,
+};
+const struct hypfs_funcs hypfs_leaf_ro_funcs = {
+    .read = hypfs_read_leaf,
+    .write = hypfs_write_deny,
+};
+const struct hypfs_funcs hypfs_leaf_wr_funcs = {
+    .read = hypfs_read_leaf,
+    .write = hypfs_write_leaf,
+};
+const struct hypfs_funcs hypfs_bool_wr_funcs = {
+    .read = hypfs_read_leaf,
+    .write = hypfs_write_bool,
+};
+const struct hypfs_funcs hypfs_custom_wr_funcs = {
+    .read = hypfs_read_leaf,
+    .write = hypfs_write_custom,
+};
+
 static DEFINE_RWLOCK(hypfs_lock);
 enum hypfs_lock_state {
     hypfs_unlocked,
@@ -74,6 +95,9 @@  static int add_entry(struct hypfs_entry_dir *parent, struct hypfs_entry *new)
     int ret = -ENOENT;
     struct hypfs_entry *e;
 
+    ASSERT(new->funcs->read);
+    ASSERT(new->funcs->write);
+
     hypfs_write_lock();
 
     list_for_each_entry ( e, &parent->dirlist, list )
@@ -284,7 +308,7 @@  static int hypfs_read(const struct hypfs_entry *entry,
 
     guest_handle_add_offset(uaddr, sizeof(e));
 
-    ret = entry->read(entry, uaddr);
+    ret = entry->funcs->read(entry, uaddr);
 
  out:
     return ret;
@@ -297,6 +321,7 @@  int hypfs_write_leaf(struct hypfs_entry_leaf *leaf,
     int ret;
 
     ASSERT(this_cpu(hypfs_locked) == hypfs_write_locked);
+    ASSERT(leaf->e.max_size);
 
     if ( ulen > leaf->e.max_size )
         return -ENOSPC;
@@ -357,6 +382,7 @@  int hypfs_write_custom(struct hypfs_entry_leaf *leaf,
     int ret;
 
     ASSERT(this_cpu(hypfs_locked) == hypfs_write_locked);
+    ASSERT(leaf->e.max_size);
 
     /* Avoid oversized buffer allocation. */
     if ( ulen > MAX_PARAM_SIZE )
@@ -382,19 +408,20 @@  int hypfs_write_custom(struct hypfs_entry_leaf *leaf,
     return ret;
 }
 
+int hypfs_write_deny(struct hypfs_entry_leaf *leaf,
+                     XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned int ulen)
+{
+    return -EACCES;
+}
+
 static int hypfs_write(struct hypfs_entry *entry,
                        XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
 {
     struct hypfs_entry_leaf *l;
 
-    if ( !entry->write )
-        return -EACCES;
-
-    ASSERT(entry->max_size);
-
     l = container_of(entry, struct hypfs_entry_leaf, e);
 
-    return entry->write(l, uaddr, ulen);
+    return entry->funcs->write(l, uaddr, ulen);
 }
 
 long do_hypfs_op(unsigned int cmd,
diff --git a/xen/include/xen/hypfs.h b/xen/include/xen/hypfs.h
index 5ad99cb558..25fdf3ead7 100644
--- a/xen/include/xen/hypfs.h
+++ b/xen/include/xen/hypfs.h
@@ -7,6 +7,32 @@ 
 #include <public/hypfs.h>
 
 struct hypfs_entry_leaf;
+struct hypfs_entry;
+
+/*
+ * Per-node callbacks:
+ *
+ * The callbacks are always called with the hypfs lock held.
+ *
+ * The read() callback is used to return the contents of a node (either
+ * directory or leaf). It is NOT used to get directory entries during traversal
+ * of the tree.
+ *
+ * The write() callback is used to modify the contents of a node. Writing
+ * directories is not supported (this means all nodes are added at boot time).
+ */
+struct hypfs_funcs {
+    int (*read)(const struct hypfs_entry *entry,
+                XEN_GUEST_HANDLE_PARAM(void) uaddr);
+    int (*write)(struct hypfs_entry_leaf *leaf,
+                 XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned int ulen);
+};
+
+extern const struct hypfs_funcs hypfs_dir_funcs;
+extern const struct hypfs_funcs hypfs_leaf_ro_funcs;
+extern const struct hypfs_funcs hypfs_leaf_wr_funcs;
+extern const struct hypfs_funcs hypfs_bool_wr_funcs;
+extern const struct hypfs_funcs hypfs_custom_wr_funcs;
 
 struct hypfs_entry {
     unsigned short type;
@@ -15,10 +41,7 @@  struct hypfs_entry {
     unsigned int max_size;
     const char *name;
     struct list_head list;
-    int (*read)(const struct hypfs_entry *entry,
-                XEN_GUEST_HANDLE_PARAM(void) uaddr);
-    int (*write)(struct hypfs_entry_leaf *leaf,
-                 XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned int ulen);
+    const struct hypfs_funcs *funcs;
 };
 
 struct hypfs_entry_leaf {
@@ -42,7 +65,7 @@  struct hypfs_entry_dir {
         .e.size = 0,                              \
         .e.max_size = 0,                          \
         .e.list = LIST_HEAD_INIT(var.e.list),     \
-        .e.read = hypfs_read_dir,                 \
+        .e.funcs = &hypfs_dir_funcs,              \
         .dirlist = LIST_HEAD_INIT(var.dirlist),   \
     }
 
@@ -52,7 +75,7 @@  struct hypfs_entry_dir {
         .e.encoding = XEN_HYPFS_ENC_PLAIN,        \
         .e.name = (nam),                          \
         .e.max_size = (msz),                      \
-        .e.read = hypfs_read_leaf,                \
+        .e.funcs = &hypfs_leaf_ro_funcs,          \
     }
 
 /* Content and size need to be set via hypfs_string_set_reference(). */
@@ -72,35 +95,37 @@  static inline void hypfs_string_set_reference(struct hypfs_entry_leaf *leaf,
     leaf->e.size = strlen(str) + 1;
 }
 
-#define HYPFS_FIXEDSIZE_INIT(var, typ, nam, contvar, wr) \
-    struct hypfs_entry_leaf __read_mostly var = {        \
-        .e.type = (typ),                                 \
-        .e.encoding = XEN_HYPFS_ENC_PLAIN,               \
-        .e.name = (nam),                                 \
-        .e.size = sizeof(contvar),                       \
-        .e.max_size = (wr) ? sizeof(contvar) : 0,        \
-        .e.read = hypfs_read_leaf,                       \
-        .e.write = (wr),                                 \
-        .u.content = &(contvar),                         \
+#define HYPFS_FIXEDSIZE_INIT(var, typ, nam, contvar, fn, wr) \
+    struct hypfs_entry_leaf __read_mostly var = {            \
+        .e.type = (typ),                                     \
+        .e.encoding = XEN_HYPFS_ENC_PLAIN,                   \
+        .e.name = (nam),                                     \
+        .e.size = sizeof(contvar),                           \
+        .e.max_size = (wr) ? sizeof(contvar) : 0,            \
+        .e.funcs = (fn),                                     \
+        .u.content = &(contvar),                             \
     }
 
 #define HYPFS_UINT_INIT(var, nam, contvar)                       \
-    HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_UINT, nam, contvar, NULL)
+    HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_UINT, nam, contvar, \
+                         &hypfs_leaf_ro_funcs, 0)
 #define HYPFS_UINT_INIT_WRITABLE(var, nam, contvar)              \
     HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_UINT, nam, contvar, \
-                         hypfs_write_leaf)
+                         &hypfs_leaf_wr_funcs, 1)
 
 #define HYPFS_INT_INIT(var, nam, contvar)                        \
-    HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_INT, nam, contvar, NULL)
+    HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_INT, nam, contvar,  \
+                         &hypfs_leaf_ro_funcs, 0)
 #define HYPFS_INT_INIT_WRITABLE(var, nam, contvar)               \
     HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_INT, nam, contvar, \
-                         hypfs_write_leaf)
+                         &hypfs_leaf_wr_funcs, 1)
 
 #define HYPFS_BOOL_INIT(var, nam, contvar)                       \
-    HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_BOOL, nam, contvar, NULL)
+    HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_BOOL, nam, contvar, \
+                         &hypfs_leaf_ro_funcs, 0)
 #define HYPFS_BOOL_INIT_WRITABLE(var, nam, contvar)              \
     HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_BOOL, nam, contvar, \
-                         hypfs_write_bool)
+                         &hypfs_bool_wr_funcs, 1)
 
 extern struct hypfs_entry_dir hypfs_root;
 
@@ -112,6 +137,8 @@  int hypfs_read_dir(const struct hypfs_entry *entry,
                    XEN_GUEST_HANDLE_PARAM(void) uaddr);
 int hypfs_read_leaf(const struct hypfs_entry *entry,
                     XEN_GUEST_HANDLE_PARAM(void) uaddr);
+int hypfs_write_deny(struct hypfs_entry_leaf *leaf,
+                     XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned int ulen);
 int hypfs_write_leaf(struct hypfs_entry_leaf *leaf,
                      XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned int ulen);
 int hypfs_write_bool(struct hypfs_entry_leaf *leaf,
diff --git a/xen/include/xen/param.h b/xen/include/xen/param.h
index d0409d3a0e..1b2c7db954 100644
--- a/xen/include/xen/param.h
+++ b/xen/include/xen/param.h
@@ -116,8 +116,7 @@  extern struct param_hypfs __paramhypfs_start[], __paramhypfs_end[];
         { .hypfs.e.type = XEN_HYPFS_TYPE_STRING, \
           .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
           .hypfs.e.name = (nam), \
-          .hypfs.e.read = hypfs_read_leaf, \
-          .hypfs.e.write = hypfs_write_custom, \
+          .hypfs.e.funcs = &hypfs_custom_wr_funcs, \
           .init_leaf = (initfunc), \
           .func = (variable) }
 #define boolean_runtime_only_param(nam, variable) \
@@ -127,8 +126,7 @@  extern struct param_hypfs __paramhypfs_start[], __paramhypfs_end[];
           .hypfs.e.name = (nam), \
           .hypfs.e.size = sizeof(variable), \
           .hypfs.e.max_size = sizeof(variable), \
-          .hypfs.e.read = hypfs_read_leaf, \
-          .hypfs.e.write = hypfs_write_bool, \
+          .hypfs.e.funcs = &hypfs_bool_wr_funcs, \
           .hypfs.u.content = &(variable) }
 #define integer_runtime_only_param(nam, variable) \
     __paramfs __parfs_##variable = \
@@ -137,8 +135,7 @@  extern struct param_hypfs __paramhypfs_start[], __paramhypfs_end[];
           .hypfs.e.name = (nam), \
           .hypfs.e.size = sizeof(variable), \
           .hypfs.e.max_size = sizeof(variable), \
-          .hypfs.e.read = hypfs_read_leaf, \
-          .hypfs.e.write = hypfs_write_leaf, \
+          .hypfs.e.funcs = &hypfs_leaf_wr_funcs, \
           .hypfs.u.content = &(variable) }
 #define size_runtime_only_param(nam, variable) \
     __paramfs __parfs_##variable = \
@@ -147,8 +144,7 @@  extern struct param_hypfs __paramhypfs_start[], __paramhypfs_end[];
           .hypfs.e.name = (nam), \
           .hypfs.e.size = sizeof(variable), \
           .hypfs.e.max_size = sizeof(variable), \
-          .hypfs.e.read = hypfs_read_leaf, \
-          .hypfs.e.write = hypfs_write_leaf, \
+          .hypfs.e.funcs = &hypfs_leaf_wr_funcs, \
           .hypfs.u.content = &(variable) }
 #define string_runtime_only_param(nam, variable) \
     __paramfs __parfs_##variable = \
@@ -157,8 +153,7 @@  extern struct param_hypfs __paramhypfs_start[], __paramhypfs_end[];
           .hypfs.e.name = (nam), \
           .hypfs.e.size = 0, \
           .hypfs.e.max_size = sizeof(variable), \
-          .hypfs.e.read = hypfs_read_leaf, \
-          .hypfs.e.write = hypfs_write_leaf, \
+          .hypfs.e.funcs = &hypfs_leaf_wr_funcs, \
           .hypfs.u.content = &(variable) }
 
 #else