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>

drivers/gpu/drm/drm_auth.c

@@ -1,18 +1,13 @@
-/**
- * \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
  *
  * Copyright 1999 Precision Insight, Inc., Cedar Park, Texas.
  * 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;
 }

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);
 }
 

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);

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;

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;
 };