[Xen-devel] [patch] ACM ops interface

[Hollis Blanchard <hollisb@xxxxxxxxxx>]

[Resend due to xense-devel list borkage.]

Hi, I noticed a few small issues in the ACM code.

First, it isn't using XEN_GUEST_HANDLEs where it should (i.e. when using
pointers in the interface). What's a little scary is that because it's
using void* everywhere, we didn't get any compiler warnings (if you're
passing buffers, maybe 'unsigned char *' would be a better type?).

Second, copy_to/from_user() has been replaced with copy_to/from_guest(),
since you can't copy to "userspace" on all architectures. (To complicate
things, there are a couple areas where x86 code actually wants to copy
to a virtual address, but copy_to/from_user() is only valid in
arch-specific code.)

Third, using 'enum' in the interface makes me very nervous. I'm not a C
lawyer, but using an explicitly-sized type would make me a lot happier.

Finally, a request: It would simplify PowerPC code if the ACM ops passed
around a union that contains every ACM struct, like 'struct dom0_op'.
It's a little hard to explain, but if you're curious, have a look at
xenppc_privcmd_dom0_op() in
http://xenbits.xensource.com/ext/linux-ppc-2.6.hg?f=aa55dca4028a;file=arch/powerpc/platforms/xen/hcall.c
 . To support the current ACM ops, we would have to define our own union 
anyways:
        
        union {
                setpolicy;
                getpolicy;
                ...
        } op;
        
        switch (cmd) {
                case SETPOLICY:
                        if (copy_from_user(&op, arg, sizeof(struct 
acm_setpolicy)))
                                return -EFAULT;
                        break;
                case SETPOLICY:
                        if (copy_from_user(&op, arg, sizeof(struct 
acm_setpolicy)))
                                return -EFAULT;
                        break;
                ...
        }
        
        __u32 *version = (__u32 *)&op;
        if (*version != ACM_VERSION)
                return -EACCES;
        
        switch (cmd) {
                case SETPOLICY:
                        /* munge acm_setpolicy */
                case GETPOLICY:
                        /* munge acm_getpolicy */
                ...
        }

If the ACM ops were always passed in a union, we could replace the first
switch with a single copy_from_user(), and also guarantee that
"interface_version" is always in the right place (it's only there
coincidentally now). What do you think?

Anyways, this compile-tested patch addresses the first three issues I
mentioned. Please add your Signed-off-by and submit upstream if you're
happy with it.

Thanks!

Signed-off-by: Hollis Blanchard <hollisb@xxxxxxxxxx>

diff -r 5c726b5ab92b tools/python/xen/lowlevel/acm/acm.c
--- a/tools/python/xen/lowlevel/acm/acm.c       Tue Jun 06 15:30:06 2006 -0500
+++ b/tools/python/xen/lowlevel/acm/acm.c       Wed Jun 07 12:31:55 2006 -0500
@@ -52,7 +52,7 @@ void * __getssid(int domid, uint32_t *bu
     }
     memset(buf, 0, SSID_BUFFER_SIZE);
     getssid.interface_version = ACM_INTERFACE_VERSION;
-    getssid.ssidbuf = buf;
+    set_xen_guest_handle(getssid.ssidbuf, buf);
     getssid.ssidbuf_size = SSID_BUFFER_SIZE;
     getssid.get_ssid_by = DOMAINID;
     getssid.id.domainid = domid;
diff -r 5c726b5ab92b xen/acm/acm_policy.c
--- a/xen/acm/acm_policy.c      Tue Jun 06 15:30:06 2006 -0500
+++ b/xen/acm/acm_policy.c      Wed Jun 07 12:31:55 2006 -0500
@@ -32,7 +32,7 @@
 #include <acm/acm_endian.h>
 
 int
-acm_set_policy(void *buf, u32 buf_size, int isuserbuffer)
+acm_set_policy(XEN_GUEST_HANDLE(void) buf, u32 buf_size, int isuserbuffer)
 {
     u8 *policy_buffer = NULL;
     struct acm_policy_buffer *pol;
@@ -45,7 +45,7 @@ acm_set_policy(void *buf, u32 buf_size, 
         return -ENOMEM;
 
     if (isuserbuffer) {
-        if (copy_from_user(policy_buffer, buf, buf_size))
+        if (copy_from_guest(policy_buffer, buf, buf_size))
         {
             printk("%s: Error copying!\n",__func__);
             goto error_free;
@@ -116,7 +116,7 @@ acm_set_policy(void *buf, u32 buf_size, 
 }
 
 int
-acm_get_policy(void *buf, u32 buf_size)
+acm_get_policy(XEN_GUEST_HANDLE(void) buf, u32 buf_size)
 { 
     u8 *policy_buffer;
     int ret;
@@ -162,7 +162,7 @@ acm_get_policy(void *buf, u32 buf_size)
         goto error_free_unlock;
 
     bin_pol->len = htonl(ntohl(bin_pol->len) + ret);
-    if (copy_to_user(buf, policy_buffer, ntohl(bin_pol->len)))
+    if (copy_to_guest(buf, policy_buffer, ntohl(bin_pol->len)))
         goto error_free_unlock;
 
     read_unlock(&acm_bin_pol_rwlock);
@@ -177,7 +177,7 @@ acm_get_policy(void *buf, u32 buf_size)
 }
 
 int
-acm_dump_statistics(void *buf, u16 buf_size)
+acm_dump_statistics(XEN_GUEST_HANDLE(void) buf, u16 buf_size)
 { 
     /* send stats to user space */
     u8 *stats_buffer;
@@ -208,7 +208,7 @@ acm_dump_statistics(void *buf, u16 buf_s
 
     memcpy(stats_buffer, &acm_stats, sizeof(struct acm_stats_buffer));
 
-    if (copy_to_user(buf, stats_buffer, sizeof(struct acm_stats_buffer) + len1 
+ len2))
+    if (copy_to_guest(buf, stats_buffer, sizeof(struct acm_stats_buffer) + 
len1 + len2))
         goto error_lock_free;
 
     read_unlock(&acm_bin_pol_rwlock);
@@ -223,7 +223,7 @@ acm_dump_statistics(void *buf, u16 buf_s
 

 int
-acm_get_ssid(ssidref_t ssidref, u8 *buf, u16 buf_size)
+acm_get_ssid(ssidref_t ssidref, XEN_GUEST_HANDLE(void) buf, u16 buf_size)
 {
     /* send stats to user space */
     u8 *ssid_buffer;
@@ -272,7 +272,7 @@ acm_get_ssid(ssidref_t ssidref, u8 *buf,
     acm_ssid->len += ret;
     acm_ssid->secondary_max_types = ret;
 
-    if (copy_to_user(buf, ssid_buffer, acm_ssid->len))
+    if (copy_to_guest(buf, ssid_buffer, acm_ssid->len))
         goto error_free_unlock;
 
     read_unlock(&acm_bin_pol_rwlock);
diff -r 5c726b5ab92b xen/include/public/acm_ops.h
--- a/xen/include/public/acm_ops.h      Tue Jun 06 15:30:06 2006 -0500
+++ b/xen/include/public/acm_ops.h      Wed Jun 07 12:31:55 2006 -0500
@@ -17,7 +17,7 @@
  * This makes sure that old versions of acm tools will stop working in a
  * well-defined way (rather than crashing the machine, for instance).
  */
-#define ACM_INTERFACE_VERSION   0xAAAA0007
+#define ACM_INTERFACE_VERSION   0xAAAA0008
 
 /************************************************************************/
 
@@ -33,7 +33,7 @@ struct acm_setpolicy {
 struct acm_setpolicy {
     /* IN */
     uint32_t interface_version;
-    void *pushcache;
+    XEN_GUEST_HANDLE(void) pushcache;
     uint32_t pushcache_size;
 };
 
@@ -42,7 +42,7 @@ struct acm_getpolicy {
 struct acm_getpolicy {
     /* IN */
     uint32_t interface_version;
-    void *pullcache;
+    XEN_GUEST_HANDLE(void) pullcache;
     uint32_t pullcache_size;
 };
 
@@ -51,7 +51,7 @@ struct acm_dumpstats {
 struct acm_dumpstats {
     /* IN */
     uint32_t interface_version;
-    void *pullcache;
+    XEN_GUEST_HANDLE(void) pullcache;
     uint32_t pullcache_size;
 };
 
@@ -61,12 +61,12 @@ struct acm_getssid {
 struct acm_getssid {
     /* IN */
     uint32_t interface_version;
-    enum get_type get_ssid_by;
+    uint32_t get_ssid_by;
     union {
         domaintype_t domainid;
         ssidref_t    ssidref;
     } id;
-    void *ssidbuf;
+    XEN_GUEST_HANDLE(void) ssidbuf;
     uint32_t ssidbuf_size;
 };
 
@@ -74,8 +74,8 @@ struct acm_getdecision {
 struct acm_getdecision {
     /* IN */
     uint32_t interface_version;
-    enum get_type get_decision_by1;
-    enum get_type get_decision_by2;
+    uint32_t get_decision_by1;
+    uint32_t get_decision_by2;
     union {
         domaintype_t domainid;
         ssidref_t    ssidref;
@@ -84,9 +84,9 @@ struct acm_getdecision {
         domaintype_t domainid;
         ssidref_t    ssidref;
     } id2;
-    enum acm_hook_type hook;
+    uint32_t hook;
     /* OUT */
-    int acm_decision;
+    uint32_t acm_decision;
 };
 
 #endif /* __XEN_PUBLIC_ACM_OPS_H__ */


-- 
Hollis Blanchard
IBM Linux Technology Center


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel