[Xen-devel] Re: [patch] ACM ops interface

[Reiner Sailer <sailer@xxxxxxxxxx>]

hollisb@xxxxxxxxxxxxxxxxxxxxxxx wrote on 06/08/2006
11:01:17 AM:

> [Resend due to xense-devel list borkage.]
> 
> Hi, I noticed a few small issues in the ACM code.

Thanks for looking into them!!

> 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?).

Keir responded to this point already

> 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.)

Seems a sensible thing to do. This part of the patch
makes much sense.

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

We should define constants and a fixed type argument
(e.g. 32bit as you suggest in your patch). You are right that this interface
benefits from clearly defined in its types and sizes.

> 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

We just changed away from this for many reasons. It
is at any time pretty clear to which structure the void pointer points.
See Keir's e-mail.

Thanks
Reiner

> 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