Re: is avoiding compat ioctls possible?

[Dave Airlie <airlied@gmail.com>]

On Fri, Oct 30, 2009 at 8:13 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 30 October 2009, Dave Airlie wrote:
>> Btw when I mentioned ioctls I meant more than radeon, all the KMS
>> ioctls in the common drm_crtc.c file suffer from this problem as well.
>>
>> Hence why I still believe either my drm specific inline or something
>> more generic (granted I can see why a generic solution would be ugly).
>>
>> You patch below does suffer from a lot of #ifdefs and cut-n-paste
>> that is a lot better suited to doing in an inline or macro. We can then
>> comment that inline saying if anyone else does this we will be most
>> unhappy.
>
> I think it would be better to do a conversion of the pointers in
> a separate compat handler, but then call the regular function, which
> in case of drm already take a kernel pointer. That would be much simpler
> than the compat_alloc_user_space tricks in the current code and
> also cleaner than trying to handle both cases in one function using
> is_compat_task().
>
> See the (not even compile-tested) example below as an illustration
> of what I think you should do. If you convert all functions in
> drm_ioc32.c to this scheme, you can replace drm_compat_ioctl and
> drm_compat_ioctls[] with drm_generic_compat_ioctl.
>

I just got back out from F12 push to look at lkml again ;-)

My main problem which no-one seem to address with all these compat ioctls
is they suck for maintainance, adding the compat_ptr "hacks" into the actual
ioctls is much easier to maintain in that I can see in the code where we've done
these and if code flow has to change I can deal with it all in one place.

Doing all these out-of-line hidden over here in a separate file just
seems crazy,
and I'm  having trouble wondering why ppl consider it a better idea at all.

It adds probably 1000 more LOC versus one inline with a decent name used
inline to replace current casting in the driver.

If the ioctl flow or interface "changes" someone has to remember about all
of these (granted this is unlikely since we pretty much set them in stone).

Dave.

>        Arnd <><
>
> diff --git a/drivers/gpu/drm/drm_ioc32.c b/drivers/gpu/drm/drm_ioc32.c
> index 282d9fd..334345b 100644
> --- a/drivers/gpu/drm/drm_ioc32.c
> +++ b/drivers/gpu/drm/drm_ioc32.c
> @@ -1040,6 +1040,122 @@ static int compat_drm_wait_vblank(struct file *file, unsigned int cmd,
>        return 0;
>  }
>
> +static int compat_drm_mode_getblob_ioctl(struct drm_device *dev,
> +                       struct drm_mode_get_blob __user *out_resp_user,
> +                       struct drm_file *file_priv)
> +{
> +       struct drm_mode_get_blob out_resp;
> +       int ret;
> +
> +       ret = copy_from_user(&out_resp, out_resp_user, sizeof(out_resp))
> +       if (ret)
> +               return -EFAULT;
> +
> +       out_resp.data = (unsigned long)compat_ptr(out_resp.data);
> +
> +       ret = drm_mode_getblob_ioctl(dev, &out_resp, file_priv);
> +       if (ret)
> +               return ret;
> +
> +       ret = copy_to_user(out_resp_user, &out_resp, sizeof(out_resp))
> +       if (ret)
> +               return -EFAULT;
> +       return 0;
> +}
> +
> +static int compat_drm_mode_gamma_set_ioctl(struct drm_device *dev,
> +                       struct drm_mode_crtc_lut __user *crtc_lut_user,
> +                       struct drm_file *file_priv)
> +{
> +       struct drm_mode_crtc_lut crtc_lut;
> +
> +       ret = copy_from_user(&crtc_lut, crtc_lut_user, sizeof(crtc_lut))
> +       if (ret)
> +               return -EFAULT;
> +
> +       crtc_lut.red   = (unsigned long)compat_ptr(crtc_lut.red);
> +       crtc_lut.green = (unsigned long)compat_ptr(crtc_lut.green);
> +       crtc_lut.blue  = (unsigned long)compat_ptr(crtc_lut.blue);
> +
> +       ret = drm_mode_gamma_set_ioctl(dev, &crtc_lut, file_priv);
> +
> +       return ret;
> +}
> +
> +static int compat_drm_mode_gamma_get_ioctl(struct drm_device *dev,
> +                       struct drm_mode_crtc_lut __user *crtc_lut_user,
> +                       struct drm_file *file_priv)
> +{
> +       struct drm_mode_crtc_lut crtc_lut;
> +
> +       ret = copy_from_user(&crtc_lut, crtc_lut_user, sizeof(crtc_lut))
> +       if (ret)
> +               return -EFAULT;
> +
> +       crtc_lut.red   = (unsigned long)compat_ptr(crtc_lut.red);
> +       crtc_lut.green = (unsigned long)compat_ptr(crtc_lut.green);
> +       crtc_lut.blue  = (unsigned long)compat_ptr(crtc_lut.blue);
> +
> +       ret = drm_mode_gamma_get_ioctl(dev, &crtc_lut, file_priv);
> +
> +       return ret;
> +}
> +
> +static int drm_generic_compat_ioctl(struct file *filp, unsigned int cmd,
> +                              unsigned long arg)
> +{
> +       struct drm_file *file_priv = filp->private_data;
> +       struct drm_device *dev = file_priv->minor->dev;
> +       unsigned int nr = DRM_IOCTL_NR(cmd);
> +       int ret;
> +
> +       atomic_inc(&dev->ioctl_count);
> +       atomic_inc(&dev->counts[_DRM_STAT_IOCTLS]);
> +       ++file_priv->ioctl_count;
> +
> +       if ((nr >= DRM_CORE_IOCTL_COUNT) &&
> +           ((nr < DRM_COMMAND_BASE) || (nr >= DRM_COMMAND_END)))
> +               goto err_i1;
> +       if ((nr >= DRM_COMMAND_BASE) && (nr < DRM_COMMAND_END) &&
> +           (nr < DRM_COMMAND_BASE + dev->driver->num_ioctls))
> +               ioctl = &dev->driver->ioctls[nr - DRM_COMMAND_BASE];
> +       else if ((nr >= DRM_COMMAND_END) || (nr < DRM_COMMAND_BASE)) {
> +               ioctl = &drm_ioctls[nr];
> +               cmd = ioctl->cmd;
> +       } else
> +               goto err_i1;
> +
> +       if (((ioctl->flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)) ||
> +                  ((ioctl->flags & DRM_AUTH) && !file_priv->authenticated) ||
> +                  ((ioctl->flags & DRM_MASTER) && !file_priv->is_master) ||
> +                  (!(ioctl->flags & DRM_CONTROL_ALLOW) && (file_priv->minor->type == DRM_MINOR_CONTROL))) {
> +               ret = -EACCES;
> +               goto err_il;
> +       }
> +
> +       switch (cmd) {
> +       case DRM_IOCTL_MODE_GETPROPBLOB:
> +               ret = compat_drm_mode_getblob_ioctl(dev,
> +                                       compat_ptr(arg), file_priv);
> +               break;
> +
> +       case DRM_IOCTL_MODE_GETGAMMA:
> +               ret = compat_drm_mode_gamma_set_ioctl(dev,
> +                                       compat_ptr(arg), file_priv);
> +               break;
> +
> +       case DRM_IOCTL_MODE_GETGAMMA:
> +               ret = compat_drm_mode_gamma_get_ioctl(dev,
> +                                       compat_ptr(arg), file_priv);
> +               break;
> +       }
> +
> +      err_i1:
> +       atomic_dec(&dev->ioctl_count);
> +
> +       return ret;
> +}
> +


>  /**
>