On Tue, Jan 05, 2010 at 12:52:04PM +0000, Jan Beulich wrote:
> - sizeof(struct privcmd_mmapbatch_32) was wrong
> - MFN array must be translated for IOCTL_PRIVCMD_MMAPBATCH
>
> Also, the error indicator of IOCTL_PRIVCMD_MMAPBATCH should be in the
> top nibble (it is documented that way in include/xen/public/privcmd.h
> and include/xen/compat_ioctl.h), but since that is an incompatible
> change it is not being done here (instead, a new ioctl with proper
> behavior will need to be added).
>
> As usual, written against 2.6.32.2 and made apply to the 2.6.18
> tree without further testing.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxxxx>
>
> --- head-2010年01月04日.orig/drivers/xen/privcmd/compat_privcmd.c 2009年11月06日 
> 10:45:48.000000000 +0100
> +++ head-2010年01月04日/drivers/xen/privcmd/compat_privcmd.c 2010年01月04日 
> 13:50:00.000000000 +0100
> @@ -51,17 +51,49 @@ int privcmd_ioctl_32(int fd, unsigned in
> struct privcmd_mmapbatch *p;
> struct privcmd_mmapbatch_32 *p32;
> struct privcmd_mmapbatch_32 n32;
> +#ifdef xen_pfn32_t
> + xen_pfn_t *__user arr;
> + xen_pfn32_t *__user arr32;
> + unsigned int i;
> +#endif
>
> p32 = compat_ptr(arg);
> p = compat_alloc_user_space(sizeof(*p));
> if (copy_from_user(&n32, p32, sizeof(n32)) ||
> put_user(n32.num, &p->num) ||
> put_user(n32.dom, &p->dom) ||
> - put_user(n32.addr, &p->addr) ||
> - put_user(compat_ptr(n32.arr), &p->arr))
> + put_user(n32.addr, &p->addr))
> return -EFAULT;
> +#ifdef xen_pfn32_t
> + arr = compat_alloc_user_space(n32.num * sizeof(*arr)
> + + sizeof(*p));
> + arr32 = compat_ptr(n32.arr);
> + for (i = 0; i < n32.num; ++i) {
> + xen_pfn32_t mfn;
> +
> + if (get_user(mfn, arr32 + i) || put_user(mfn, arr + i))
> + return -EFAULT;
> + }
> +
> + if (put_user(arr, &p->arr))
> + return -EFAULT;
> +#else
> + if (put_user(compat_ptr(n32.arr), &p->arr))
> + return -EFAULT;
> +#endif
>
> ret = sys_ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH, (unsigned long)p);
> +
> +#ifdef xen_pfn32_t
> + for (i = 0; !ret && i < n32.num; ++i) {
> + xen_pfn_t mfn;
> +
> + if (get_user(mfn, arr + i) || put_user(mfn, arr32 + i))
> + ret = -EFAULT;
> + else if (mfn != (xen_pfn32_t)mfn)
> + ret = -ERANGE;
> + }
> +#endif
> }
> break;
> default:
Perhaps this could be refactored to reduce or remove the #ifdefs ?
> --- head-2010年01月04日.orig/include/xen/compat_ioctl.h 2007年07月10日 
> 09:42:30.000000000 +0200
> +++ head-2010年01月04日/include/xen/compat_ioctl.h 2009年12月17日 
> 15:40:40.000000000 +0100
> @@ -23,6 +23,11 @@
> #define __LINUX_XEN_COMPAT_H__ 
>
> #include <linux/compat.h>
> +#include <linux/compiler.h>
> +
> +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> +#define xen_pfn32_t __u32
> +#endif
Is it usual in xen for a type to be a #define in xen?
Ito my mind it would make things a lot clearer to use a #define called
SOME_FEATURE and then either use __u32 directly for the type or
typedef __u32 xen_pfn32_t.
>
> extern int privcmd_ioctl_32(int fd, unsigned int cmd, unsigned long arg);
> struct privcmd_mmap_32 {
> @@ -34,7 +39,14 @@ struct privcmd_mmap_32 {
> struct privcmd_mmapbatch_32 {
> int num; /* number of pages to populate */
> domid_t dom; /* target domain */
> +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> + union { /* virtual address */
> + __u64 addr __packed;
> + __u32 va;
> + };
> +#else
> __u64 addr; /* virtual address */
> +#endif
> compat_uptr_t arr; /* array of mfns - top nibble set on err */
> };
> #define IOCTL_PRIVCMD_MMAP_32 \
Its unclear to me why va is necessary.
It doesn't seem to be used anywhere in the patch.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel