drm: simplify authentication management

The magic auth tokens we have are a simple map from cyclic IDs to drm_file
objects. Remove all the old bulk of code and replace it with a simple,
direct IDR.

The previous behavior is kept. Especially calling authmagic multiple times
on the same magic results in EINVAL except on the first call. The only
difference in behavior is that we never allocate IDs multiple times as
long as a client has its FD open.

v2:
 - Fix return code of GetMagic()
 - Use non-cyclic IDR allocator
 - fix off-by-one in "magic > INT_MAX" sanity check

v3:
 - drop redundant "magic > INT_MAX" check

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 8a37524d0867de8bdc2bf864919ef45a5f12156e..50d0baa06db057057341dce5dfab930782785510 100644 (file)
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -1,11 +1,3 @@
-/**
- * \file drm_auth.c
- * IOCTLs for authentication
- *
- * \author Rickard E. (Rik) Faith <faith@valinux.com>
- * \author Gareth Hughes <gareth@valinux.com>
- */
-
 /*
  * Created: Tue Feb  2 08:37:54 1999 by faith@valinux.com
  *
@@ -13,6 +5,9 @@
  * Copyright 2000 VA Linux Systems, Inc., Sunnyvale, California.
  * All Rights Reserved.
  *
+ * Author Rickard E. (Rik) Faith <faith@valinux.com>
+ * Author Gareth Hughes <gareth@valinux.com>
+ *
  * Permission is hereby granted, free of charge, to any person obtaining a
  * copy of this software and associated documentation files (the "Software"),
  * to deal in the Software without restriction, including without limitation
@@ -36,151 +31,47 @@
 #include <drm/drmP.h>
 #include "drm_internal.h"
 
-struct drm_magic_entry {
-       struct drm_hash_item hash_item;
-       struct drm_file *priv;
-};
-
-/**
- * Find the file with the given magic number.
- *
- * \param dev DRM device.
- * \param magic magic number.
- *
- * Searches in drm_device::magiclist within all files with the same hash key
- * the one with matching magic number, while holding the drm_device::struct_mutex
- * lock.
- */
-static struct drm_file *drm_find_file(struct drm_master *master, drm_magic_t magic)
-{
-       struct drm_file *retval = NULL;
-       struct drm_magic_entry *pt;
-       struct drm_hash_item *hash;
-       struct drm_device *dev = master->minor->dev;
-
-       mutex_lock(&dev->struct_mutex);
-       if (!drm_ht_find_item(&master->magiclist, (unsigned long)magic, &hash)) {
-               pt = drm_hash_entry(hash, struct drm_magic_entry, hash_item);
-               retval = pt->priv;
-       }
-       mutex_unlock(&dev->struct_mutex);
-       return retval;
-}
-
-/**
- * Adds a magic number.
- *
- * \param dev DRM device.
- * \param priv file private data.
- * \param magic magic number.
- *
- * Creates a drm_magic_entry structure and appends to the linked list
- * associated the magic number hash key in drm_device::magiclist, while holding
- * the drm_device::struct_mutex lock.
- */
-static int drm_add_magic(struct drm_master *master, struct drm_file *priv,
-                        drm_magic_t magic)
-{
-       struct drm_magic_entry *entry;
-       struct drm_device *dev = master->minor->dev;
-       DRM_DEBUG("%d\n", magic);
-
-       entry = kzalloc(sizeof(*entry), GFP_KERNEL);
-       if (!entry)
-               return -ENOMEM;
-       entry->priv = priv;
-       entry->hash_item.key = (unsigned long)magic;
-       mutex_lock(&dev->struct_mutex);
-       drm_ht_insert_item(&master->magiclist, &entry->hash_item);
-       mutex_unlock(&dev->struct_mutex);
-
-       return 0;
-}
-
-/**
- * Remove a magic number.
- *
- * \param dev DRM device.
- * \param magic magic number.
- *
- * Searches and unlinks the entry in drm_device::magiclist with the magic
- * number hash key, while holding the drm_device::struct_mutex lock.
- */
-int drm_remove_magic(struct drm_master *master, drm_magic_t magic)
-{
-       struct drm_magic_entry *pt;
-       struct drm_hash_item *hash;
-       struct drm_device *dev = master->minor->dev;
-
-       DRM_DEBUG("%d\n", magic);
-
-       mutex_lock(&dev->struct_mutex);
-       if (drm_ht_find_item(&master->magiclist, (unsigned long)magic, &hash)) {
-               mutex_unlock(&dev->struct_mutex);
-               return -EINVAL;
-       }
-       pt = drm_hash_entry(hash, struct drm_magic_entry, hash_item);
-       drm_ht_remove_item(&master->magiclist, hash);
-       mutex_unlock(&dev->struct_mutex);
-
-       kfree(pt);
-
-       return 0;
-}
-
 /**
- * Get a unique magic number (ioctl).
+ * drm_getmagic - Get unique magic of a client
+ * @dev: DRM device to operate on
+ * @data: ioctl data containing the drm_auth object
+ * @file_priv: DRM file that performs the operation
  *
- * \param inode device inode.
- * \param file_priv DRM file private.
- * \param cmd command.
- * \param arg pointer to a resulting drm_auth structure.
- * \return zero on success, or a negative number on failure.
+ * This looks up the unique magic of the passed client and returns it. If the
+ * client did not have a magic assigned, yet, a new one is registered. The magic
+ * is stored in the passed drm_auth object.
  *
- * If there is a magic number in drm_file::magic then use it, otherwise
- * searches an unique non-zero magic number and add it associating it with \p
- * file_priv.
- * This ioctl needs protection by the drm_global_mutex, which protects
- * struct drm_file::magic and struct drm_magic_entry::priv.
+ * Returns: 0 on success, negative error code on failure.
  */
 int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
 {
-       static drm_magic_t sequence = 0;
-       static DEFINE_SPINLOCK(lock);
        struct drm_auth *auth = data;
+       int ret = 0;
 
-       /* Find unique magic */
-       if (file_priv->magic) {
-               auth->magic = file_priv->magic;
-       } else {
-               do {
-                       spin_lock(&lock);
-                       if (!sequence)
-                               ++sequence;     /* reserve 0 */
-                       auth->magic = sequence++;
-                       spin_unlock(&lock);
-               } while (drm_find_file(file_priv->master, auth->magic));
-               file_priv->magic = auth->magic;
-               drm_add_magic(file_priv->master, file_priv, auth->magic);
+       mutex_lock(&dev->struct_mutex);
+       if (!file_priv->magic) {
+               ret = idr_alloc(&file_priv->master->magic_map, file_priv,
+                               1, 0, GFP_KERNEL);
+               if (ret >= 0)
+                       file_priv->magic = ret;
        }
+       auth->magic = file_priv->magic;
+       mutex_unlock(&dev->struct_mutex);
 
        DRM_DEBUG("%u\n", auth->magic);
 
-       return 0;
+       return ret < 0 ? ret : 0;
 }
 
 /**
- * Authenticate with a magic.
+ * drm_authmagic - Authenticate client with a magic
+ * @dev: DRM device to operate on
+ * @data: ioctl data containing the drm_auth object
+ * @file_priv: DRM file that performs the operation
  *
- * \param inode device inode.
- * \param file_priv DRM file private.
- * \param cmd command.
- * \param arg pointer to a drm_auth structure.
- * \return zero if authentication successed, or a negative number otherwise.
+ * This looks up a DRM client by the passed magic and authenticates it.
  *
- * Checks if \p file_priv is associated with the magic number passed in \arg.
- * This ioctl needs protection by the drm_global_mutex, which protects
- * struct drm_file::magic and struct drm_magic_entry::priv.
+ * Returns: 0 on success, negative error code on failure.
  */
 int drm_authmagic(struct drm_device *dev, void *data,
                  struct drm_file *file_priv)
@@ -189,10 +80,14 @@ int drm_authmagic(struct drm_device *dev, void *data,
        struct drm_file *file;
 
        DRM_DEBUG("%u\n", auth->magic);
-       if ((file = drm_find_file(file_priv->master, auth->magic))) {
+
+       mutex_lock(&dev->struct_mutex);
+       file = idr_find(&file_priv->master->magic_map, auth->magic);
+       if (file) {
                file->authenticated = 1;
-               drm_remove_magic(file_priv->master, auth->magic);
-               return 0;
+               idr_replace(&file_priv->master->magic_map, NULL, auth->magic);
        }
-       return -EINVAL;
+       mutex_unlock(&dev->struct_mutex);
+
+       return file ? 0 : -EINVAL;
 }

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 26ed9feb3cfd88092c27b211543b0dea36e8d76a..88b594c0593abcf464c4cf3d366385d7a37b58cc 100644 (file)
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -92,8 +92,6 @@ void drm_ut_debug_printk(const char *function_name, const char *format, ...)
 }
 EXPORT_SYMBOL(drm_ut_debug_printk);
 
-#define DRM_MAGIC_HASH_ORDER  4  /**< Size of key hash table. Must be power of 2. */
-
 struct drm_master *drm_master_create(struct drm_minor *minor)
 {
        struct drm_master *master;
@@ -105,10 +103,7 @@ struct drm_master *drm_master_create(struct drm_minor *minor)
        kref_init(&master->refcount);
        spin_lock_init(&master->lock.spinlock);
        init_waitqueue_head(&master->lock.lock_queue);
-       if (drm_ht_create(&master->magiclist, DRM_MAGIC_HASH_ORDER)) {
-               kfree(master);
-               return NULL;
-       }
+       idr_init(&master->magic_map);
        master->minor = minor;
 
        return master;
@@ -143,10 +138,9 @@ static void drm_master_destroy(struct kref *kref)
                master->unique = NULL;
                master->unique_len = 0;
        }
-
-       drm_ht_remove(&master->magiclist);
-
        mutex_unlock(&dev->struct_mutex);
+
+       idr_destroy(&master->magic_map);
        kfree(master);
 }
 

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 076dd606b58080681e1cfe5a646448ee3ac46854..0f6a5c8801e3ec716d1ae4a802bfb09221b13286 100644 (file)
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -380,6 +380,8 @@ int drm_release(struct inode *inode, struct file *filp)
 
        mutex_lock(&dev->struct_mutex);
        list_del(&file_priv->lhead);
+       if (file_priv->magic)
+               idr_remove(&file_priv->master->magic_map, file_priv->magic);
        mutex_unlock(&dev->struct_mutex);
 
        if (dev->driver->preclose)
@@ -394,11 +396,6 @@ int drm_release(struct inode *inode, struct file *filp)
                  (long)old_encode_dev(file_priv->minor->kdev->devt),
                  dev->open_count);
 
-       /* Release any auth tokens that might point to this file_priv,
-          (do that under the drm_global_mutex) */
-       if (file_priv->magic)
-               (void) drm_remove_magic(file_priv->master, file_priv->magic);
-
        /* if the master has gone away we can't do anything with the lock */
        if (file_priv->minor->master)
                drm_master_release(dev, filp);

diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 12a61d70682785a1078e5f398064d3a4dd659b10..059af01bd07a9916e560b772e4a5db872583a54b 100644 (file)
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -69,7 +69,6 @@ int drm_getmagic(struct drm_device *dev, void *data,
                 struct drm_file *file_priv);
 int drm_authmagic(struct drm_device *dev, void *data,
                  struct drm_file *file_priv);
-int drm_remove_magic(struct drm_master *master, drm_magic_t magic);
 
 /* drm_sysfs.c */
 extern struct class *drm_class;

diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index e2b101b49112834955ab8804b79cf212eb99431c..df6d9970d9a4abb04f6e9b13e78f5842344fec07 100644 (file)
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -355,7 +355,7 @@ struct drm_lock_data {
  * @minor: Link back to minor char device we are master for. Immutable.
  * @unique: Unique identifier: e.g. busid. Protected by drm_global_mutex.
  * @unique_len: Length of unique field. Protected by drm_global_mutex.
- * @magiclist: Hash of used authentication tokens. Protected by struct_mutex.
+ * @magic_map: Map of used authentication tokens. Protected by struct_mutex.
  * @lock: DRI lock information.
  * @driver_priv: Pointer to driver-private information.
  */
@@ -364,7 +364,7 @@ struct drm_master {
        struct drm_minor *minor;
        char *unique;
        int unique_len;
-       struct drm_open_hash magiclist;
+       struct idr magic_map;
        struct drm_lock_data lock;
        void *driver_priv;
 };