[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@us.ibm.com>
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@lists.xensource.com
http://lists.xensource.com/xen-devel
On 8 Jun 2006, at 16:01, Hollis Blanchard wrote:> 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?The interface was deliberately changed away from that style for consistency with other hypercalls. Also to allow larger argument structures to be added to the interface later without breaking backward compatibility (inflating the size of the union). -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Thu, 2006-06-08 at 17:11 +0100, Keir Fraser wrote:> On 8 Jun 2006, at 16:01, Hollis Blanchard wrote: > > > 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? > > The interface was deliberately changed away from that style for > consistency with other hypercalls. Also to allow larger argument > structures to be added to the interface later without breaking backward > compatibility (inflating the size of the union).Ah, that makes sense. Too bad it will uglify the already unpleasant handle hacks PPC needs to do. Are you planning to do something similar for the dom0_op structure? -- Hollis Blanchard IBM Linux Technology Center _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
hollisb@us.ltcfwd.linux.ibm.com 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() inWe 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@us.ibm.com> > > 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, intisuserbuffer)> { > 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, u16buf_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 ina> * 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@lists.xensource.com http://lists.xensource.com/xen-devel
On Thu, 2006-06-08 at 14:17 -0400, Reiner Sailer wrote:> > > > 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 alreadyI didn''t see that, actually. -- Hollis Blanchard IBM Linux Technology Center _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 8 Jun 2006, at 18:59, Hollis Blanchard wrote:> Ah, that makes sense. Too bad it will uglify the already unpleasant > handle hacks PPC needs to do. > > Are you planning to do something similar for the dom0_op structure?No, it''s too big so too much hassle. Also I added padding to it some time ago so the union is hopefully big enough for future use and, if not, we can always link out to a secondary structure for some subcommands. So it''s the only remaining union-style hypercall and it''s here to stay. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 8 Jun 2006, at 19:32, Hollis Blanchard wrote:>>> 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 > > I didn''t see that, actually.Yeah, I don''t think I did. Anyway, should I take Hollis''s patch or do you want it worked on some more? As for copy_to/from_user sleeping, they don''t sleep in Xen but it would be nice to avoid putting it in a locked region anyway in case we want to do anything clever in future. I may start asserting that in debug builds. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
* Reiner Sailer (sailer@us.ibm.com) wrote:> > 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.While you''re at it, can you fix the things doing copy_to/from with locks held? Those could sleep. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
* Keir Fraser (Keir.Fraser@cl.cam.ac.uk) wrote:> As for copy_to/from_user sleeping, they don''t sleep in Xen but it would > be nice to avoid putting it in a locked region anyway in case we want > to do anything clever in future. I may start asserting that in debug > builds.OK, that sounds good. -chris _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser <Keir.Fraser@cl.cam.ac.uk> wrote on 06/08/2006 02:42:10 PM:> > On 8 Jun 2006, at 19:32, Hollis Blanchard wrote: > > >>> 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 > > > > I didn''t see that, actually. > > Yeah, I don''t think I did. > > Anyway, should I take Hollis''s patch or do you want it worked on some > more? > > As for copy_to/from_user sleeping, they don''t sleep in Xen but it would > be nice to avoid putting it in a locked region anyway in case we want > to do anything clever in future. I may start asserting that in debug > builds. > > -- Keir >Makes sense. If Hollis doesn''t include it in his patch, then I will address it next week. Reiner _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 8 Jun 2006, at 22:25, Reiner Sailer wrote:> > Anyway, should I take Hollis''s patch or do you want it worked on > some > > more? > > > > As for copy_to/from_user sleeping, they don''t sleep in Xen but it > would > > be nice to avoid putting it in a locked region anyway in case we > want > > to do anything clever in future. I may start asserting that in debug > > builds. > > > > -- Keir > > > > Makes sense. > > If Hollis doesn''t include it in his patch, then I will address it next > week.Looking at how broken some other hypercalls would be if I added this restriction (e.g., dom0_op) don''t worry about it. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel