[PATCH 1/3] drm: drop unused 'magicfree' list

[PATCH 1/3] drm: drop unused 'magicfree' list

[David Herrmann]

This list is write-only. It's never used for read-access, so no reason to
keep it around. Drop it!

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_auth.c | 3 ---
 drivers/gpu/drm/drm_drv.c  | 1 -
 include/drm/drmP.h         | 2 --
 3 files changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index fc8e8aa..8a37524 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -37,7 +37,6 @@
 #include "drm_internal.h"
 
 struct drm_magic_entry {
-	struct list_head head;
 	struct drm_hash_item hash_item;
 	struct drm_file *priv;
 };
@@ -93,7 +92,6 @@ static int drm_add_magic(struct drm_master *master, struct drm_file *priv,
 	entry->hash_item.key = (unsigned long)magic;
 	mutex_lock(&dev->struct_mutex);
 	drm_ht_insert_item(&master->magiclist, &entry->hash_item);
-	list_add_tail(&entry->head, &master->magicfree);
 	mutex_unlock(&dev->struct_mutex);
 
 	return 0;
@@ -123,7 +121,6 @@ int drm_remove_magic(struct drm_master *master, drm_magic_t magic)
 	}
 	pt = drm_hash_entry(hash, struct drm_magic_entry, hash_item);
 	drm_ht_remove_item(&master->magiclist, hash);
-	list_del(&pt->head);
 	mutex_unlock(&dev->struct_mutex);
 
 	kfree(pt);
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 48f7359..26ed9fe 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -109,7 +109,6 @@ struct drm_master *drm_master_create(struct drm_minor *minor)
 		kfree(master);
 		return NULL;
 	}
-	INIT_LIST_HEAD(&master->magicfree);
 	master->minor = minor;
 
 	return master;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 62c40777..80be07a 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -356,7 +356,6 @@ struct drm_lock_data {
  * @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.
- * @magicfree: List of used authentication tokens. Protected by struct_mutex.
  * @lock: DRI lock information.
  * @driver_priv: Pointer to driver-private information.
  */
@@ -366,7 +365,6 @@ struct drm_master {
 	char *unique;
 	int unique_len;
 	struct drm_open_hash magiclist;
-	struct list_head magicfree;
 	struct drm_lock_data lock;
 	void *driver_priv;
 };
-- 
2.3.7

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm: simplify authentication management

[David Herrmann]

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, even
if they were already authenticated. To trigger that, you need 2^31 open
DRM file descriptions at the same time, though.

Diff-stat:
   5 files changed, 45 insertions(+), 157 deletions(-)

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_auth.c     | 178 +++++++++--------------------------------
 drivers/gpu/drm/drm_drv.c      |  12 +--
 drivers/gpu/drm/drm_fops.c     |   7 +-
 drivers/gpu/drm/drm_internal.h |   1 -
 include/drm/drmP.h             |   4 +-
 5 files changed, 45 insertions(+), 157 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 8a37524..ee365e8 100644
--- 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.
+ * 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 dev DRM device.
- * \param priv file private data.
- * \param magic magic number.
+ * 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.
  *
- * 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).
- *
- * \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.
- *
- * 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_cyclic(&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;
 }
 
 /**
- * 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,17 @@ 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))) {
+
+	if (auth->magic >= INT_MAX)
+		return -EINVAL;
+
+	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 26ed9fe..88b594c 100644
--- 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 076dd60..0f6a5c8 100644
--- 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 12a61d7..059af01 100644
--- 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 80be07a..2f9d605 100644
--- 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;
 };
-- 
2.3.7

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm: simplify master cleanup

[David Herrmann]

In drm_master_destroy() we _free_ the master object. There is no reason to
hold any locks while dropping its static members, nor do we have to reset
it to 0.

Furthermore, kfree() already does NULL checks, so call it directly on
master->unique and drop the redundant reset-code.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_drv.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 88b594c..3b3c4f5 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -132,15 +132,10 @@ static void drm_master_destroy(struct kref *kref)
 			r_list = NULL;
 		}
 	}
-
-	if (master->unique) {
-		kfree(master->unique);
-		master->unique = NULL;
-		master->unique_len = 0;
-	}
 	mutex_unlock(&dev->struct_mutex);
 
 	idr_destroy(&master->magic_map);
+	kfree(master->unique);
 	kfree(master);
 }
 
-- 
2.3.7

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: drop unused 'magicfree' list

[Chris Wilson]

On Mon, May 04, 2015 at 04:05:12PM +0200, David Herrmann wrote:
> This list is write-only. It's never used for read-access, so no reason to
> keep it around. Drop it!
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: simplify authentication management

[Chris Wilson]

On Mon, May 04, 2015 at 04:05:13PM +0200, David Herrmann wrote:
> 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, even
> if they were already authenticated. To trigger that, you need 2^31 open
> DRM file descriptions at the same time, though.
> 
> Diff-stat:
>    5 files changed, 45 insertions(+), 157 deletions(-)
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

Just one quibble,

> @@ -189,10 +80,17 @@ 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))) {
> +
> +	if (auth->magic >= INT_MAX)
> +		return -EINVAL;

The idr upper bound is INT_MAX [inclusive].

> +	mutex_lock(&dev->struct_mutex);
> +	file = idr_find(&file_priv->master->magic_map, auth->magic);

But given that it will return NULL anyway, do you really want to
short-circuit the erroneous lookup, and just leave it to the idr to
validate it itself?

So... How efficient is the cyclic idr for a long running system that has
accumulated several hundred thousand stale magics?

Maybe an igt to create a couple of billion shortlived clients (with
overlapping lifetimes) and check the behaviour?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm: simplify master cleanup

[Chris Wilson]

On Mon, May 04, 2015 at 04:05:14PM +0200, David Herrmann wrote:
> In drm_master_destroy() we _free_ the master object. There is no reason to
> hold any locks while dropping its static members, nor do we have to reset
> it to 0.
> 
> Furthermore, kfree() already does NULL checks, so call it directly on
> master->unique and drop the redundant reset-code.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: simplify authentication management

[David Herrmann]

Hi

On Mon, May 4, 2015 at 4:46 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, May 04, 2015 at 04:05:13PM +0200, David Herrmann wrote:
>> 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, even
>> if they were already authenticated. To trigger that, you need 2^31 open
>> DRM file descriptions at the same time, though.
>>
>> Diff-stat:
>>    5 files changed, 45 insertions(+), 157 deletions(-)
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>
> Just one quibble,
>
>> @@ -189,10 +80,17 @@ 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))) {
>> +
>> +     if (auth->magic >= INT_MAX)
>> +             return -EINVAL;
>
> The idr upper bound is INT_MAX [inclusive].
>
>> +     mutex_lock(&dev->struct_mutex);
>> +     file = idr_find(&file_priv->master->magic_map, auth->magic);
>
> But given that it will return NULL anyway, do you really want to
> short-circuit the erroneous lookup, and just leave it to the idr to
> validate it itself?

Indeed, I can just change it to "auth->magic > INT_MAX". Fixed!

> So... How efficient is the cyclic idr for a long running system that has
> accumulated several hundred thousand stale magics?
>
> Maybe an igt to create a couple of billion shortlived clients (with
> overlapping lifetimes) and check the behaviour?

I'm not sure this is an interesting benchmark. I mean, you usually
have a dozen DRM clients max, anyway. So the only interesting detail
is whether the cyclic behavior causes the IDR tree to degenerate. But
in that case, we should either fix IDR or see whether we can avoid the
cyclic behavior.

Given that I use idr_replace(idr, NULL, id) on auth, instead of an
immediate idr_remove(), we can just use idr_alloc() instead of
idr_alloc_cyclic(). However, idr_alloc_cyclic() seems to work fine for
inotify, so it should be fine for auth-magics, I guess (and
auth-magics aren't part of any fast-path, right?).

Anyway, I can ping Tejun and ask whether the cyclic IDR has any
visible downsides compared to normal allocation. He should know.

Thanks!
David
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: simplify authentication management

[Chris Wilson]

On Mon, May 04, 2015 at 05:02:37PM +0200, David Herrmann wrote:
> Hi
> 
> On Mon, May 4, 2015 at 4:46 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Mon, May 04, 2015 at 04:05:13PM +0200, David Herrmann wrote:
> >> 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, even
> >> if they were already authenticated. To trigger that, you need 2^31 open
> >> DRM file descriptions at the same time, though.
> >>
> >> Diff-stat:
> >>    5 files changed, 45 insertions(+), 157 deletions(-)
> >>
> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> >
> > Just one quibble,
> >
> >> @@ -189,10 +80,17 @@ 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))) {
> >> +
> >> +     if (auth->magic >= INT_MAX)
> >> +             return -EINVAL;
> >
> > The idr upper bound is INT_MAX [inclusive].
> >
> >> +     mutex_lock(&dev->struct_mutex);
> >> +     file = idr_find(&file_priv->master->magic_map, auth->magic);
> >
> > But given that it will return NULL anyway, do you really want to
> > short-circuit the erroneous lookup, and just leave it to the idr to
> > validate it itself?
> 
> Indeed, I can just change it to "auth->magic > INT_MAX". Fixed!
> 
> > So... How efficient is the cyclic idr for a long running system that has
> > accumulated several hundred thousand stale magics?
> >
> > Maybe an igt to create a couple of billion shortlived clients (with
> > overlapping lifetimes) and check the behaviour?
> 
> I'm not sure this is an interesting benchmark. I mean, you usually
> have a dozen DRM clients max, anyway. So the only interesting detail
> is whether the cyclic behavior causes the IDR tree to degenerate. But
> in that case, we should either fix IDR or see whether we can avoid the
> cyclic behavior.

I was actually more concerned about what happens if we end up with 2
billion stale clients - do we get 2 billion entries? Can we even
allocate that many? I suspect we end up with a DoS.
 
> Given that I use idr_replace(idr, NULL, id) on auth, instead of an
> immediate idr_remove(), we can just use idr_alloc() instead of
> idr_alloc_cyclic(). However, idr_alloc_cyclic() seems to work fine for
> inotify, so it should be fine for auth-magics, I guess (and
> auth-magics aren't part of any fast-path, right?).

Right, not concerned about speed here. Presuming no anomalous behaviour!
 
> Anyway, I can ping Tejun and ask whether the cyclic IDR has any
> visible downsides compared to normal allocation. He should know.

Anyway, I just thought the test would be reasonably easy to write and
useful to compare behaviour of long running systems - and helps to
improve our test coverage just that little bit.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: simplify authentication management

[David Herrmann]

Hi

On Mon, May 4, 2015 at 5:11 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, May 04, 2015 at 05:02:37PM +0200, David Herrmann wrote:
>> Hi
>>
>> On Mon, May 4, 2015 at 4:46 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > On Mon, May 04, 2015 at 04:05:13PM +0200, David Herrmann wrote:
>> >> 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, even
>> >> if they were already authenticated. To trigger that, you need 2^31 open
>> >> DRM file descriptions at the same time, though.
>> >>
>> >> Diff-stat:
>> >>    5 files changed, 45 insertions(+), 157 deletions(-)
>> >>
>> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> >
>> > Just one quibble,
>> >
>> >> @@ -189,10 +80,17 @@ 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))) {
>> >> +
>> >> +     if (auth->magic >= INT_MAX)
>> >> +             return -EINVAL;
>> >
>> > The idr upper bound is INT_MAX [inclusive].
>> >
>> >> +     mutex_lock(&dev->struct_mutex);
>> >> +     file = idr_find(&file_priv->master->magic_map, auth->magic);
>> >
>> > But given that it will return NULL anyway, do you really want to
>> > short-circuit the erroneous lookup, and just leave it to the idr to
>> > validate it itself?
>>
>> Indeed, I can just change it to "auth->magic > INT_MAX". Fixed!
>>
>> > So... How efficient is the cyclic idr for a long running system that has
>> > accumulated several hundred thousand stale magics?
>> >
>> > Maybe an igt to create a couple of billion shortlived clients (with
>> > overlapping lifetimes) and check the behaviour?
>>
>> I'm not sure this is an interesting benchmark. I mean, you usually
>> have a dozen DRM clients max, anyway. So the only interesting detail
>> is whether the cyclic behavior causes the IDR tree to degenerate. But
>> in that case, we should either fix IDR or see whether we can avoid the
>> cyclic behavior.
>
> I was actually more concerned about what happens if we end up with 2
> billion stale clients - do we get 2 billion entries? Can we even
> allocate that many? I suspect we end up with a DoS.

Ehh, the magic-entries are dropped on close(). So to actually keep 2
billion entries, you also need 2 billion "struct file*", "struct
drm_file*", ...

The idr_alloc_cyclic() does _not_ mark old entries as "used". All it
does is try to remember new IDs in a cyclic order. On wrap-around, old
IDs will get re-used.

Thanks
David
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: simplify authentication management

[Chris Wilson]

On Mon, May 04, 2015 at 05:14:51PM +0200, David Herrmann wrote:
> Hi
> 
> On Mon, May 4, 2015 at 5:11 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Mon, May 04, 2015 at 05:02:37PM +0200, David Herrmann wrote:
> >> Hi
> >>
> >> On Mon, May 4, 2015 at 4:46 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >> > On Mon, May 04, 2015 at 04:05:13PM +0200, David Herrmann wrote:
> >> >> 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, even
> >> >> if they were already authenticated. To trigger that, you need 2^31 open
> >> >> DRM file descriptions at the same time, though.
> >> >>
> >> >> Diff-stat:
> >> >>    5 files changed, 45 insertions(+), 157 deletions(-)
> >> >>
> >> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> >> >
> >> > Just one quibble,
> >> >
> >> >> @@ -189,10 +80,17 @@ 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))) {
> >> >> +
> >> >> +     if (auth->magic >= INT_MAX)
> >> >> +             return -EINVAL;
> >> >
> >> > The idr upper bound is INT_MAX [inclusive].
> >> >
> >> >> +     mutex_lock(&dev->struct_mutex);
> >> >> +     file = idr_find(&file_priv->master->magic_map, auth->magic);
> >> >
> >> > But given that it will return NULL anyway, do you really want to
> >> > short-circuit the erroneous lookup, and just leave it to the idr to
> >> > validate it itself?
> >>
> >> Indeed, I can just change it to "auth->magic > INT_MAX". Fixed!
> >>
> >> > So... How efficient is the cyclic idr for a long running system that has
> >> > accumulated several hundred thousand stale magics?
> >> >
> >> > Maybe an igt to create a couple of billion shortlived clients (with
> >> > overlapping lifetimes) and check the behaviour?
> >>
> >> I'm not sure this is an interesting benchmark. I mean, you usually
> >> have a dozen DRM clients max, anyway. So the only interesting detail
> >> is whether the cyclic behavior causes the IDR tree to degenerate. But
> >> in that case, we should either fix IDR or see whether we can avoid the
> >> cyclic behavior.
> >
> > I was actually more concerned about what happens if we end up with 2
> > billion stale clients - do we get 2 billion entries? Can we even
> > allocate that many? I suspect we end up with a DoS.
> 
> Ehh, the magic-entries are dropped on close(). So to actually keep 2
> billion entries, you also need 2 billion "struct file*", "struct
> drm_file*", ...
> 
> The idr_alloc_cyclic() does _not_ mark old entries as "used". All it
> does is try to remember new IDs in a cyclic order. On wrap-around, old
> IDs will get re-used.

But the layers are only freed if all below them are empty (iiui).
Basically, I am not entirely familar with how idr will cope on a long
running system, and would like some elaboration (and a test for future
reference!).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: simplify authentication management

[David Herrmann]

Hi

On Mon, May 4, 2015 at 5:25 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, May 04, 2015 at 05:14:51PM +0200, David Herrmann wrote:
>> Hi
>>
>> On Mon, May 4, 2015 at 5:11 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > On Mon, May 04, 2015 at 05:02:37PM +0200, David Herrmann wrote:
>> >> Hi
>> >>
>> >> On Mon, May 4, 2015 at 4:46 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> >> > On Mon, May 04, 2015 at 04:05:13PM +0200, David Herrmann wrote:
>> >> >> 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, even
>> >> >> if they were already authenticated. To trigger that, you need 2^31 open
>> >> >> DRM file descriptions at the same time, though.
>> >> >>
>> >> >> Diff-stat:
>> >> >>    5 files changed, 45 insertions(+), 157 deletions(-)
>> >> >>
>> >> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> >> >
>> >> > Just one quibble,
>> >> >
>> >> >> @@ -189,10 +80,17 @@ 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))) {
>> >> >> +
>> >> >> +     if (auth->magic >= INT_MAX)
>> >> >> +             return -EINVAL;
>> >> >
>> >> > The idr upper bound is INT_MAX [inclusive].
>> >> >
>> >> >> +     mutex_lock(&dev->struct_mutex);
>> >> >> +     file = idr_find(&file_priv->master->magic_map, auth->magic);
>> >> >
>> >> > But given that it will return NULL anyway, do you really want to
>> >> > short-circuit the erroneous lookup, and just leave it to the idr to
>> >> > validate it itself?
>> >>
>> >> Indeed, I can just change it to "auth->magic > INT_MAX". Fixed!
>> >>
>> >> > So... How efficient is the cyclic idr for a long running system that has
>> >> > accumulated several hundred thousand stale magics?
>> >> >
>> >> > Maybe an igt to create a couple of billion shortlived clients (with
>> >> > overlapping lifetimes) and check the behaviour?
>> >>
>> >> I'm not sure this is an interesting benchmark. I mean, you usually
>> >> have a dozen DRM clients max, anyway. So the only interesting detail
>> >> is whether the cyclic behavior causes the IDR tree to degenerate. But
>> >> in that case, we should either fix IDR or see whether we can avoid the
>> >> cyclic behavior.
>> >
>> > I was actually more concerned about what happens if we end up with 2
>> > billion stale clients - do we get 2 billion entries? Can we even
>> > allocate that many? I suspect we end up with a DoS.
>>
>> Ehh, the magic-entries are dropped on close(). So to actually keep 2
>> billion entries, you also need 2 billion "struct file*", "struct
>> drm_file*", ...
>>
>> The idr_alloc_cyclic() does _not_ mark old entries as "used". All it
>> does is try to remember new IDs in a cyclic order. On wrap-around, old
>> IDs will get re-used.
>
> But the layers are only freed if all below them are empty (iiui).
> Basically, I am not entirely familar with how idr will cope on a long
> running system, and would like some elaboration (and a test for future
> reference!).

IDR uses a tree of idr_layer objects (basically an optimized suffix
tree on the binary ID). Tree depth is increased dynamically if a layer
runs full. Max depths of layers is 4. Each layer manages 256 children
(leafs store the void* data, nodes store pointers to sub-layers). Size
of a layer is roughly 2k on 64bit.

If the ID-range is small and packed, the storage is used optimally.
But a sparse ID-range might occupy one layer per ID in worst case.
Lookup speed should be negligible as we are limited to a depth of 4
layers. However, memory consumption might get huge if the IDs are
spread even. But this really depends on the allocation scheme.

I'm not sure how to write a benchmark for this. I mean, I can easily
craft one that causes the IDR to degenerate, but it requires us to
keep huge numbers of files open.
But this fact makes IDR rather suboptimal for cyclic allocations, so I
switched to idr_alloc() now. This guarantees tight/packed ID ranges
and user-space cannot degenerate the layers, anymore. That is, unless
you open more than 256 FDs on a device in parallel, we're fine with a
single IDR layer; always. This should work fine, right?

Thanks
David
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: simplify authentication management

[Chris Wilson]

On Mon, May 04, 2015 at 06:28:25PM +0200, David Herrmann wrote:
> I'm not sure how to write a benchmark for this. I mean, I can easily
> craft one that causes the IDR to degenerate, but it requires us to
> keep huge numbers of files open.
> But this fact makes IDR rather suboptimal for cyclic allocations, so I
> switched to idr_alloc() now. This guarantees tight/packed ID ranges
> and user-space cannot degenerate the layers, anymore. That is, unless
> you open more than 256 FDs on a device in parallel, we're fine with a
> single IDR layer; always. This should work fine, right?

That pretty much circumvents my only worry! If there is a client leak,
the system will eventually keel under the load, and that we have a huge
number of magics open is insignificant.

As far as test coverage I would focus on

(a) authenticating up to vfs-file-max fds (i.e. check that we hit
the system limits without authmagic failing)

(b) churn through a small number of clients for a few minutes, just
basically checking for anomalous behaviour and that allocation times
every minute or so remain constant.

(c) just check that we can authenticate! always useful for patches that
touch the authmagic system

I was thinking of a few more, but they basically serve to show the holes
in the authmagic scheme.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/3] drm: simplify authentication management

[David Herrmann]

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.

Diff-stat:
   5 files changed, 45 insertions(+), 157 deletions(-)

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
v2:
 - Fix return code of GetMagic()
 - Use non-cyclic IDR allocator
 - fix off-by-one in "magic > INT_MAX" sanity check

 drivers/gpu/drm/drm_auth.c     | 178 +++++++++--------------------------------
 drivers/gpu/drm/drm_drv.c      |  12 +--
 drivers/gpu/drm/drm_fops.c     |   7 +-
 drivers/gpu/drm/drm_internal.h |   1 -
 include/drm/drmP.h             |   4 +-
 5 files changed, 45 insertions(+), 157 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 8a37524..f5f77db 100644
--- 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.
+ * 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 dev DRM device.
- * \param priv file private data.
- * \param magic magic number.
+ * 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.
  *
- * 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).
- *
- * \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.
- *
- * 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,17 @@ 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))) {
+
+	if (auth->magic > INT_MAX)
+		return -EINVAL;
+
+	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 26ed9fe..88b594c 100644
--- 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 076dd60..0f6a5c8 100644
--- 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 12a61d7..059af01 100644
--- 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 80be07a..2f9d605 100644
--- 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;
 };
-- 
2.3.7

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: simplify authentication management

[David Herrmann]

[-- Attachment #1: Type: text/plain, Size: 1566 bytes --]

Hi

On Mon, May 4, 2015 at 6:49 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, May 04, 2015 at 06:28:25PM +0200, David Herrmann wrote:
>> I'm not sure how to write a benchmark for this. I mean, I can easily
>> craft one that causes the IDR to degenerate, but it requires us to
>> keep huge numbers of files open.
>> But this fact makes IDR rather suboptimal for cyclic allocations, so I
>> switched to idr_alloc() now. This guarantees tight/packed ID ranges
>> and user-space cannot degenerate the layers, anymore. That is, unless
>> you open more than 256 FDs on a device in parallel, we're fine with a
>> single IDR layer; always. This should work fine, right?
>
> That pretty much circumvents my only worry! If there is a client leak,
> the system will eventually keel under the load, and that we have a huge
> number of magics open is insignificant.
>
> As far as test coverage I would focus on
>
> (a) authenticating up to vfs-file-max fds (i.e. check that we hit
> the system limits without authmagic failing)
>
> (b) churn through a small number of clients for a few minutes, just
> basically checking for anomalous behaviour and that allocation times
> every minute or so remain constant.
>
> (c) just check that we can authenticate! always useful for patches that
> touch the authmagic system
>
> I was thinking of a few more, but they basically serve to show the holes
> in the authmagic scheme.

Attached is an i-g-t patch to test for basic drm-auth/magic behavior.
Comments welcome!

I also sent v2 of this patch seconds ago.

Thanks
David

[-- Attachment #2: 0001-tests-add-drm_auth-tests-for-generic-DRM-auth-magic-.patch --]
[-- Type: text/x-patch, Size: 5733 bytes --]

From 96feb83e95b2c533698ace24fe3cd25fbf114b92 Mon Sep 17 00:00:00 2001
From: David Herrmann <dh.herrmann@gmail.com>
Date: Mon, 4 May 2015 20:15:54 +0200
Subject: [PATCH] tests: add drm_auth tests for generic DRM-auth-magic testing

This adds tests/drm_auth.c which tests for drmGetMagic() and
drmAuthMagic() deficiencies.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 tests/.gitignore       |   1 +
 tests/Makefile.sources |   1 +
 tests/drm_auth.c       | 163 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 165 insertions(+)
 create mode 100644 tests/drm_auth.c

diff --git a/tests/.gitignore b/tests/.gitignore
index 796e330..50bf3eb 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -3,6 +3,7 @@ core_get_client_auth
 core_getclient
 core_getstats
 core_getversion
+drm_auth
 drm_import_export
 drm_read
 drm_vma_limiter
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 4cbc50d..2797a0f 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -91,6 +91,7 @@ TESTS_progs = \
 	core_getclient \
 	core_getstats \
 	core_getversion \
+	drm_auth \
 	drm_import_export \
 	drm_read \
 	drm_vma_limiter \
diff --git a/tests/drm_auth.c b/tests/drm_auth.c
new file mode 100644
index 0000000..3a97d68
--- /dev/null
+++ b/tests/drm_auth.c
@@ -0,0 +1,163 @@
+/*
+ * Copyright 2015 David Herrmann <dh.herrmann@gmail.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
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+/*
+ * Testcase: drmGetMagic() and drmAuthMagic()
+ */
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <string.h>
+#include <signal.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <errno.h>
+#include <sys/stat.h>
+#include <sys/ioctl.h>
+#include <sys/time.h>
+#include <sys/poll.h>
+#include "drm.h"
+#include "ioctl_wrappers.h"
+#include "drmtest.h"
+#include "igt_aux.h"
+
+IGT_TEST_DESCRIPTION("Call drmGetMagic() and drmAuthMagic() and see if it behaves.");
+
+static int magic_cmp(const void *p1, const void *p2)
+{
+	return *(const drm_magic_t*)p1 < *(const drm_magic_t*)p2;
+}
+
+static void test_many_magics(int master)
+{
+	drm_magic_t magic, *magics = NULL;
+	unsigned int i, j, ns, allocated = 0;
+	char path[512];
+	int *fds = NULL, slave;
+
+	sprintf(path, "/proc/self/fd/%d", master);
+
+	for (i = 0; ; ++i) {
+		/* open slave and make sure it's NOT a master */
+		slave = open(path, O_RDWR | O_CLOEXEC);
+		if (slave < 0) {
+			igt_assert(errno == EMFILE);
+			break;
+		}
+		igt_assert(drmSetMaster(slave) < 0);
+
+		/* resize magic-map */
+		if (i >= allocated) {
+			ns = allocated * 2;
+			igt_assert(ns >= allocated);
+
+			if (!ns)
+				ns = 128;
+
+			magics = realloc(magics, sizeof(*magics) * ns);
+			igt_assert(magics);
+
+			fds = realloc(fds, sizeof(*fds) * ns);
+			igt_assert(fds);
+
+			allocated = ns;
+		}
+
+		/* insert magic */
+		igt_assert(drmGetMagic(slave, &magic) == 0);
+		igt_assert(magic > 0);
+
+		magics[i] = magic;
+		fds[i] = slave;
+	}
+
+	/* make sure we could at least open a reasonable number of files */
+	igt_assert(i > 128);
+
+	/*
+	 * We cannot open the DRM file anymore. Lets sort the magic-map and
+	 * verify no magic was used multiple times.
+	 */
+	qsort(magics, i, sizeof(*magics), magic_cmp);
+	for (j = 1; j < i; ++j)
+		igt_assert(magics[j] != magics[j - 1]);
+
+	/* make sure we can authenticate all of them */
+	for (j = 0; j < i; ++j)
+		igt_assert(drmAuthMagic(master, magics[j]) == 0);
+
+	/* close files again */
+	for (j = 0; j < i; ++j)
+		close(fds[j]);
+
+	free(fds);
+	free(magics);
+}
+
+static void test_basic_auth(int master)
+{
+	drm_magic_t magic, old_magic;
+	int slave;
+
+	/* open slave and make sure it's NOT a master */
+	slave = drm_open_any();
+	igt_require(slave >= 0);
+	igt_require(drmSetMaster(slave) < 0);
+
+	/* retrieve magic for slave */
+	igt_assert(drmGetMagic(slave, &magic) == 0);
+	igt_assert(magic > 0);
+
+	/* verify the same magic is returned every time */
+	old_magic = magic;
+	igt_assert(drmGetMagic(slave, &magic) == 0);
+	igt_assert_eq(magic, old_magic);
+
+	/* verify magic can be authorized exactly once, on the master */
+	igt_assert(drmAuthMagic(slave, magic) < 0);
+	igt_assert(drmAuthMagic(master, magic) == 0);
+	igt_assert(drmAuthMagic(master, magic) < 0);
+
+	/* verify that the magic did not change */
+	old_magic = magic;
+	igt_assert(drmGetMagic(slave, &magic) == 0);
+	igt_assert_eq(magic, old_magic);
+
+	close(slave);
+}
+
+igt_main
+{
+	int master;
+
+	igt_fixture
+		master = drm_open_any_master();
+
+	igt_subtest("basic-auth")
+		test_basic_auth(master);
+
+	igt_subtest("many-magics")
+		test_many_magics(master);
+}
-- 
2.3.7


[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] drm: simplify authentication management

[Chris Wilson]

On Mon, May 04, 2015 at 08:19:33PM +0200, David Herrmann wrote:
> 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.
> 
> Diff-stat:
>    5 files changed, 45 insertions(+), 157 deletions(-)
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> v2:
>  - Fix return code of GetMagic()
>  - Use non-cyclic IDR allocator
>  - fix off-by-one in "magic > INT_MAX" sanity check

I'm being nitpicking because I don't understand the rationale for doing
the range check ourselves. idr_find() will return NULL quite happily if
the id wraps around to negative.

However, that's a nit and I like its simplicity (or at least hiding the
complexity in widely used lib code!),

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] drm: simplify authentication management

[David Herrmann]

Hi

On Mon, May 4, 2015 at 8:33 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, May 04, 2015 at 08:19:33PM +0200, David Herrmann wrote:
>> 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.
>>
>> Diff-stat:
>>    5 files changed, 45 insertions(+), 157 deletions(-)
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> ---
>> v2:
>>  - Fix return code of GetMagic()
>>  - Use non-cyclic IDR allocator
>>  - fix off-by-one in "magic > INT_MAX" sanity check
>
> I'm being nitpicking because I don't understand the rationale for doing
> the range check ourselves. idr_find() will return NULL quite happily if
> the id wraps around to negative.

...my bad. I thought you were talking about the off-by-one. I actually
thought drm_magic_t might be 64bit under some circumstances, so I
wanted to avoid the double-mapping. But you're right, it's just an
unsigned int so we can forward it unchanged to the idr helpers.

Fixed!

Thanks
David

> However, that's a nit and I like its simplicity (or at least hiding the
> complexity in widely used lib code!),
>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 2/3] drm: simplify authentication management

[David Herrmann]

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.

Diff-stat:
   5 files changed, 42 insertions(+), 157 deletions(-)

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
v3:
 - drop redundant "magic > INT_MAX" check

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

 drivers/gpu/drm/drm_auth.c     | 175 +++++++++--------------------------------
 drivers/gpu/drm/drm_drv.c      |  12 +--
 drivers/gpu/drm/drm_fops.c     |   7 +-
 drivers/gpu/drm/drm_internal.h |   1 -
 include/drm/drmP.h             |   4 +-
 5 files changed, 42 insertions(+), 157 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 8a37524..50d0baa 100644
--- 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 26ed9fe..88b594c 100644
--- 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 076dd60..0f6a5c8 100644
--- 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 12a61d7..059af01 100644
--- 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 80be07a..2f9d605 100644
--- 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;
 };
-- 
2.3.7

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm: simplify master cleanup

[Daniel Vetter]

On Mon, May 04, 2015 at 03:47:13PM +0100, Chris Wilson wrote:
> On Mon, May 04, 2015 at 04:05:14PM +0200, David Herrmann wrote:
> > In drm_master_destroy() we _free_ the master object. There is no reason to
> > hold any locks while dropping its static members, nor do we have to reset
> > it to 0.
> > 
> > Furthermore, kfree() already does NULL checks, so call it directly on
> > master->unique and drop the redundant reset-code.
> > 
> > Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Both kernel and igt patches applied, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel