Re: is avoiding compat ioctls possible?

Arnd Bergmann
2009-10-28T12:13:36Z

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.