lkml.org 
[lkml]   [2009]   [Oct]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: is avoiding compat ioctls possible?
    Date
    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 <><

    \
     
     \ /
      Last update: 2009-10-28 13:15    [W:0.028 / U:60.140 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site