Re: is avoiding compat ioctls possible?

[Arnd Bergmann <arndbergmann@googlemail.com>]

On Wednesday 28 October 2009, David Miller wrote:
> -/* The two 64-bit arches where alignof(u64)==4 in 32-bit code */
>  #if defined (CONFIG_X86_64) || defined(CONFIG_IA64)
>  typedef struct drm_radeon_setparam32 {
>         int param;
>         u64 value;
>  } __attribute__((packed)) drm_radeon_setparam32_t;
> +#else
> +#define drm_radeon_setparam32_t drm_radeon_setparam_t
> +#endif

I guess a cleaner way to put this would be

typedef struct drm_radeon_setparam32 {
       int param;
       compat_u64 value;
} drm_radeon_setparam32_t;

>  static int compat_radeon_cp_setparam(struct file *file, unsigned int cmd,
>                                      unsigned long arg)
>  {
>         drm_radeon_setparam32_t req32;
>         drm_radeon_setparam_t __user *request;
> +       compat_uptr_t uptr;
>  
>         if (copy_from_user(&req32, (void __user *) arg, sizeof(req32)))
>                 return -EFAULT;

The ioctl argument actually needs a compat_ptr() conversion as well.
For the s390 case, we can't do that in common code, because some
ioctl methods put a 32 bit integer into the argument. Not sure if we
want to fix that everywhere, the problem is very common and the
impact is minimal.
 
> +static int compat_radeon_info_ioctl(struct file *file, unsigned int cmd,
> +                                   unsigned long arg)
> +{
> +       struct drm_radeon_info __user *uinfo;
> +       struct drm_radeon_info kinfo;
> +       compat_uptr_t uaddr;
> +       void *uptr;
> +
> +       if (copy_from_user(&kinfo, (void __user *) arg, sizeof(kinfo)))
> +               return -EFAULT;
> +
> +       uaddr = kinfo.value;
> +       uptr = compat_ptr(uaddr);
> +       if (kinfo.value == (uint64_t) uptr)
> +               return drm_ioctl(file->f_dentry->d_inode, file,
> +                                DRM_IOCTL_RADEON_INFO, arg);
> +
> +       kinfo.value = (uint64_t) uptr;
> +
> +       uinfo = compat_alloc_user_space(sizeof(*uinfo));
> +       if (copy_to_user(uinfo, &kinfo, sizeof(kinfo)))
> +               return -EFAULT;
> +
> +       return drm_ioctl(file->f_dentry->d_inode, file,
> +                        DRM_IOCTL_RADEON_INFO, (unsigned long) uinfo);
> +}

IMHO a better way to handle the radeon specific ioctls would be to
avoid the compat_alloc_user_space and just define the common function
taking a kernel pointer, with two implementations of the copy operation:

static int __radeon_info_ioctl(struct file *file, struct drm_radeon_info *info);

static int radeon_info_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
	struct drm_radeon_info kinfo;

	if (copy_from_user(&kinfo, (void __user *)arg, sizeof(kinfo)))
		return -EFAULT;

	return __radeon_info_ioctl(file, cmd, &kinfo);
}

static int radeon_info_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
	struct compat_drm_radeon_info kinfo;

	if (copy_from_user(&kinfo, compat_ptr(arg), sizeof(kinfo)))
		return -EFAULT;

	kinfo.value = (u64)compat_ptr(kinfo.value);
	return __radeon_info_ioctl(file, cmd, &kinfo);
}

> @@ -426,13 +507,20 @@ long radeon_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  long radeon_kms_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  {
>         unsigned int nr = DRM_IOCTL_NR(cmd);
> +       drm_ioctl_compat_t *fn = NULL;
>         int ret;
>  
>         if (nr < DRM_COMMAND_BASE)
>                 return drm_compat_ioctl(filp, cmd, arg);
>  
> +       if (nr < DRM_COMMAND_BASE + DRM_ARRAY_SIZE(radeon_compat_kms_ioctls))
> +               fn = radeon_compat_kms_ioctls[nr - DRM_COMMAND_BASE];
> +
>         lock_kernel();          /* XXX for now */
> -       ret = drm_ioctl(filp->f_path.dentry->d_inode, filp, cmd, arg);
> +       if (fn != NULL)
> +               ret = (*fn) (filp, cmd, arg);
> +       else
> +               ret = drm_ioctl(filp->f_path.dentry->d_inode, filp, cmd, arg);
>         unlock_kernel();

This could consequently become

 
       if (nr < DRM_COMMAND_BASE)
                return drm_compat_ioctl(filp, cmd, arg);
 
+	switch (cmd) {
+	case DRM_IOCTL_RADEON_INFO:
+		return radeon_info_compat_ioctl(filp, cmd, arg);
+	case ...
+	}
+
        lock_kernel();          /* XXX for now */
        ret = drm_ioctl(filp->f_path.dentry->d_inode, filp, cmd, arg);
        unlock_kernel();


The other changes in your patch look good to me.

	Arnd <><