[PATCH] drm: hollow-out GET_CLIENT ioctl

[David Herrmann]

Hi

On Tue, Jul 16, 2013 at 3:14 PM, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> We not only have debugfs files to do pretty much the equivalent of
> lsof, we also have an ioctl. Not that compared to lsof this dumps a
> wee bit more information, but we can still get at that from debugfs
> easily.
>
> I've dug around in mesa, libdrm and ddx histories and the only users
> seem to be drm/tests/dristat.c and drm/tests/getclients.c. The later
> is a testcase for the ioctl itself since up to
>
> commit b018fcdaa5e8b4eabb8cffda687d00004a3c4785
> Author: Eric Anholt <eric at anholt.net>
> Date:   Thu Nov 22 18:46:54 2007 +1000
>
>     drm: Make DRM_IOCTL_GET_CLIENT return EINVAL when it can't find client #idx
>
> there was actually no way at all for userspace to enumerate all
> clients since the kernel just wouldn't tell it when to stop. Which
> completely broke it's only user, dristat -c.
>
> So obviously that ioctl wasn't much use for debugging. Hence I don't
> see any point in keeping support for a tool which was pretty obviously
> never really used, and while we have good replacements in the form of
> equivalent debugfs files.
>
> Still, to keep dristat -c from looping forever again stop it early by
> returning an unconditional -EINVAL. Also add a comment in the code
> about why.
>
> v2: Slightly less hollowed-out implementation. libva uses GET_CLIENTS
> to figure out whether the fd it has is already authenticated or not.
> So we need to keep that part of things working. Simplest way is to
> just return one entry to keep va_drm_is_authenticated in
> libva/va/drm/va_drm_auth.c working.
>
> Cc: Gwenole Beauchesne <gwenole.beauchesne at intel.com>
> Cc: David Herrmann <dh.herrmann at gmail.com>
> Cc: Eric Anholt <eric at anholt.net>
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
>  drivers/gpu/drm/drm_ioctl.c | 43 ++++++++++++++++++++++---------------------
>  1 file changed, 22 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 0acf080..ac8ca5c 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -217,29 +217,30 @@ int drm_getclient(struct drm_device *dev, void *data,
>                   struct drm_file *file_priv)
>  {
>         struct drm_client *client = data;
> -       struct drm_file *pt;
> -       int idx;
> -       int i;
> -
> -       idx = client->idx;
> -       i = 0;
>
> -       mutex_lock(&dev->struct_mutex);
> -       list_for_each_entry(pt, &dev->filelist, lhead) {
> -               if (i++ >= idx) {
> -                       client->auth = pt->authenticated;
> -                       client->pid = pid_vnr(pt->pid);
> -                       client->uid = from_kuid_munged(current_user_ns(), pt->uid);
> -                       client->magic = pt->magic;
> -                       client->iocs = pt->ioctl_count;
> -                       mutex_unlock(&dev->struct_mutex);
> -
> -                       return 0;
> -               }
> +       /*
> +        * Hollowed-out getclient ioctl to keep some dead old drm tests/tools
> +        * not breaking completely. Userspace tools stop enumerating one they
> +        * get -EINVAL, hence this is the return value we need to hand back for
> +        * no clients tracked.
> +        *
> +        * Unfortunately some clients (*cough* libva *cough*) use this in a fun
> +        * attempt to figure out whether they're authenticated or not. Since
> +        * that's the only thing they care about, give it to the directly
> +        * instead of walking one giant list.
> +        */
> +       if (client->idx == 0) {
> +               client->auth = file_priv->authenticated;
> +               client->pid = pid_vnr(file_priv->pid);
> +               client->uid = from_kuid_munged(current_user_ns(),
> +                                              file_priv->uid);
> +               client->magic = 0;
> +               client->iocs = 0;

magic = 0 is the only thing that bugs me as "0" is an invalid
magic-number for drmAuth. I hope no-one uses it. But we can fix that
if we see any regression. I cannot imagine anyone really using it.

Reviewed-by: David Herrmann <dh.herrmann at gmail.com>

> +
> +               return 0;
> +       } else {
> +               return -EINVAL;
>         }
> -       mutex_unlock(&dev->struct_mutex);
> -
> -       return -EINVAL;
>  }
>
>  /**
> --
> 1.8.3.2
>