drm: drop unused 'magicfree' list
Subsystems (2)
PATCH 1/3 drm: drop unused 'magicfree' list
This list is write-only. It's never used for read-access, so no reason to keep it around. Drop it!
--- drivers/gpu/drm/drm_auth.c | 3 --- drivers/gpu/drm/drm_drv.c | 1 - include/drm/drmP.h | 2 -- 3 files changed, 6 deletions(-)
drivers/gpu/drm/drm_auth.c +0 -3
@@ -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);
drivers/gpu/drm/drm_drv.c +0 -1
@@ -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;
include/drm/drmP.h +0 -2
@@ -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; };
Signature · 6 lines
2.3.7 _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 2/3 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, 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(-)
--- 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(-)
drivers/gpu/drm/drm_auth.c +38 -140
@@ -1,11 +1,3 @@ -/** -* \file drm_auth.c -* IOCTLs for authentication -* -* \author Rickard E. (Rik) Faith <[email protected]> -* \author Gareth Hughes <[email protected]> -*/ -/* * Created: Tue Feb 2 08:37:54 1999 by [email protected] *
@@ -13,6 +5,9 @@* Copyright 2000 VA Linux Systems, Inc., Sunnyvale, California. * All Rights Reserved. * +* Author Rickard E. (Rik) Faith <[email protected]> +* Author Gareth Hughes <[email protected]> +* * 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 ; }
drivers/gpu/drm/drm_drv.c +3 -9
@@ -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 +2 -5
@@ -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 +0 -1
@@ -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 +2 -2
@@ -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; };
Signature · 6 lines
2.3.7 _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm: simplify authentication management
Quoted 10 lines · David Herrmann
The magic auth tokens we have are a simple map from cyclic IDs to
drm_fileobjects. 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.
Quoted 4 lines
Diff-stat:
5 files changed, 45 insertions(+), 157 deletions(-)Signed-off-by David Herrmann <[email protected]>
Just one quibble,
drivers/gpu/drm/drm_auth.c int drm_authmagic(struct drm_device *dev, void *data, · line 85
Quoted 8 lines
drivers/gpu/drm/drm_auth.c +3
-1@@ -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].
drivers/gpu/drm/drm_auth.c int drm_authmagic(struct drm_device *dev, void *data, · line 88
Quoted 2 lines
drivers/gpu/drm/drm_auth.c +2
-0+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
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm: simplify authentication management
Hi
Quoted 29 lines · Chris Wilson
Quoted 10 lines · David Herrmann
The magic auth tokens we have are a simple map from cyclic IDs to
drm_fileobjects. 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.
Quoted 4 lines
Diff-stat:
5 files changed, 45 insertions(+), 157 deletions(-)Signed-off-by David Herrmann <[email protected]>Just one quibble,
drivers/gpu/drm/drm_auth.c int drm_authmagic(struct drm_device *dev, void *data, · line 85
Quoted 8 lines
drivers/gpu/drm/drm_auth.c +3
-1@@ -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].
Quoted 6 lines
drivers/gpu/drm/drm_auth.c int drm_authmagic(struct drm_device *dev, void *data, · line 88
Quoted 2 lines
drivers/gpu/drm/drm_auth.c +2
-0+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!
Quoted 5 lines
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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm: simplify authentication management
Quoted 44 lines · David Herrmann
Hi
Quoted 29 lines · Chris Wilson
Quoted 10 lines · David Herrmann
The magic auth tokens we have are a simple map from cyclic IDs to
drm_fileobjects. 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.
Quoted 4 lines
Diff-stat:
5 files changed, 45 insertions(+), 157 deletions(-)Signed-off-by David Herrmann <[email protected]>Just one quibble,
drivers/gpu/drm/drm_auth.c int drm_authmagic(struct drm_device *dev, void *data, · line 85
Quoted 8 lines
drivers/gpu/drm/drm_auth.c +3
-1@@ -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].Quoted 6 lines
drivers/gpu/drm/drm_auth.c int drm_authmagic(struct drm_device *dev, void *data, · line 88
Quoted 2 lines
drivers/gpu/drm/drm_auth.c +2
-0+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!Quoted 3 lines
So... How efficient is the cyclic idr for a long running system that has accumulated several hundred thousand stale magics?
Quoted 8 lines
Quoted 2 lines
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.
Quoted 5 lines
Given that I use
idr_replace(idr, NULL, id)on auth, instead of an immediateidr_remove(), we can just useidr_alloc()instead ofidr_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!
Quoted 2 lines
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
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm: simplify authentication management
Hi
Quoted 54 lines · Chris Wilson
Quoted 44 lines · David Herrmann
Hi
Quoted 29 lines · Chris Wilson
Quoted 10 lines · David Herrmann
The magic auth tokens we have are a simple map from cyclic IDs to
drm_fileobjects. 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.
Quoted 4 lines
Diff-stat:
5 files changed, 45 insertions(+), 157 deletions(-)Signed-off-by David Herrmann <[email protected]>Just one quibble,
drivers/gpu/drm/drm_auth.c int drm_authmagic(struct drm_device *dev, void *data, · line 85
Quoted 8 lines
drivers/gpu/drm/drm_auth.c +3
-1@@ -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].Quoted 6 lines
drivers/gpu/drm/drm_auth.c int drm_authmagic(struct drm_device *dev, void *data, · line 88
Quoted 2 lines
drivers/gpu/drm/drm_auth.c +2
-0+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!Quoted 3 lines
So... How efficient is the cyclic idr for a long running system that has accumulated several hundred thousand stale magics?
Quoted 8 lines
Quoted 2 lines
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.
Quoted 3 lines
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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm: simplify authentication management
Quoted 61 lines · David Herrmann
Hi
Quoted 54 lines · Chris Wilson
Quoted 44 lines · David Herrmann
Hi
Quoted 29 lines · Chris Wilson
Quoted 10 lines · David Herrmann
The magic auth tokens we have are a simple map from cyclic IDs to
drm_fileobjects. 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.
Quoted 4 lines
Diff-stat:
5 files changed, 45 insertions(+), 157 deletions(-)Signed-off-by David Herrmann <[email protected]>Just one quibble,
drivers/gpu/drm/drm_auth.c int drm_authmagic(struct drm_device *dev, void *data, · line 85
Quoted 8 lines
drivers/gpu/drm/drm_auth.c +3
-1@@ -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].Quoted 6 lines
drivers/gpu/drm/drm_auth.c int drm_authmagic(struct drm_device *dev, void *data, · line 88
Quoted 2 lines
drivers/gpu/drm/drm_auth.c +2
-0+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!Quoted 3 lines
So... How efficient is the cyclic idr for a long running system that has accumulated several hundred thousand stale magics?
Quoted 8 lines
Quoted 2 lines
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.
Quoted 3 lines
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.
Quoted 7 lines
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
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm: simplify authentication management
Hi
Quoted 66 lines · Chris Wilson
Quoted 65 lines · David Herrmann
Hi
Quoted 54 lines · Chris Wilson
Quoted 44 lines · David Herrmann
Hi
Quoted 29 lines · Chris Wilson
Quoted 10 lines · David Herrmann
The magic auth tokens we have are a simple map from cyclic IDs to
drm_fileobjects. 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.
Quoted 4 lines
Diff-stat:
5 files changed, 45 insertions(+), 157 deletions(-)Signed-off-by David Herrmann <[email protected]>Just one quibble,
drivers/gpu/drm/drm_auth.c int drm_authmagic(struct drm_device *dev, void *data, · line 85
Quoted 8 lines
drivers/gpu/drm/drm_auth.c +3
-1@@ -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].Quoted 6 lines
drivers/gpu/drm/drm_auth.c int drm_authmagic(struct drm_device *dev, void *data, · line 88
Quoted 2 lines
drivers/gpu/drm/drm_auth.c +2
-0+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!Quoted 3 lines
So... How efficient is the cyclic idr for a long running system that has accumulated several hundred thousand stale magics?
Quoted 8 lines
Quoted 2 lines
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.
Quoted 3 lines
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*", ...
Quoted 8 lines
Quoted 3 lines
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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm: simplify authentication management
Quoted 8 lines · David Herrmann
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 toidr_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
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm: simplify authentication management
Hi
Quoted 23 lines · Chris Wilson
Quoted 8 lines · David Herrmann
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 toidr_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.
Quoted 5 lines
(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
From 96feb83e95b2c533698ace24fe3cd25fbf114b92 Mon Sep 17 00:00:00 2001
From: David Herrmann <[email protected]>
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.
--- tests/.gitignore | 1 + tests/Makefile.sources | 1 + tests/drm_auth.c | 163 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 165 insertions(+) create mode 100644 tests/drm_auth.c
tests/.gitignore +1 -0
@@ -3,6 +3,7 @@ core_get_client_auth core_getclient core_getstats core_getversion +drm_auth drm_import_export drm_read drm_vma_limiter
tests/Makefile.sources +1 -0
@@ -91,6 +91,7 @@ TESTS_progs = \ core_getclient \ core_getstats \ core_getversion \ + drm_auth \ drm_import_export \ drm_read \ drm_vma_limiter \
tests/drm_auth.c new file +163 -0
163 lines not on this page · raw message
Signature · 1 line
2.3.7
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v2 2/3 drm: simplify authentication management
In reply to David HerrmannThe 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(-)
---
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(-)
drivers/gpu/drm/drm_auth.c +38 -140
@@ -1,11 +1,3 @@ -/** -* \file drm_auth.c -* IOCTLs for authentication -* -* \author Rickard E. (Rik) Faith <[email protected]> -* \author Gareth Hughes <[email protected]> -*/ -/* * Created: Tue Feb 2 08:37:54 1999 by [email protected] *
@@ -13,6 +5,9 @@* Copyright 2000 VA Linux Systems, Inc., Sunnyvale, California. * All Rights Reserved. * +* Author Rickard E. (Rik) Faith <[email protected]> +* Author Gareth Hughes <[email protected]> +* * 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 ; }
drivers/gpu/drm/drm_drv.c +3 -9
@@ -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 +2 -5
@@ -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 +0 -1
@@ -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 +2 -2
@@ -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; };
Signature · 6 lines
2.3.7 _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 2/3] drm: simplify authentication management
Quoted 10 lines · David Herrmann
The magic auth tokens we have are a simple map from cyclic IDs to
drm_fileobjects. 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:
Quoted 8 lines
5 files changed, 45 insertions(+), 157 deletions(-)Signed-off-by David Herrmann <[email protected]>---
v2:
- Fix return code ofGetMagic()
- 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!),
-Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 2/3] drm: simplify authentication management
Hi
Quoted 20 lines · Chris Wilson
Quoted 10 lines · David Herrmann
The magic auth tokens we have are a simple map from cyclic IDs to
drm_fileobjects. 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:
Quoted 8 lines
5 files changed, 45 insertions(+), 157 deletions(-)Signed-off-by David Herrmann <[email protected]>---
v2:
- Fix return code ofGetMagic()
- Use non-cyclic IDR allocator
- fix off-by-one in "magic >INT_MAX" sanity check
Quoted 3 lines
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
Quoted 8 lines
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 <[email protected]>-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v3 2/3 drm: simplify authentication management
In reply to David HerrmannThe 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(-)
---
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(-)
drivers/gpu/drm/drm_auth.c +35 -140
@@ -1,11 +1,3 @@ -/** -* \file drm_auth.c -* IOCTLs for authentication -* -* \author Rickard E. (Rik) Faith <[email protected]> -* \author Gareth Hughes <[email protected]> -*/ -/* * Created: Tue Feb 2 08:37:54 1999 by [email protected] *
@@ -13,6 +5,9 @@* Copyright 2000 VA Linux Systems, Inc., Sunnyvale, California. * All Rights Reserved. * +* Author Rickard E. (Rik) Faith <[email protected]> +* Author Gareth Hughes <[email protected]> +* * 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 +3 -9
@@ -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 +2 -5
@@ -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 +0 -1
@@ -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 +2 -2
@@ -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; };
Signature · 6 lines
2.3.7 _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 3/3 drm: simplify master cleanup
In reply to David HerrmannIn 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.
--- drivers/gpu/drm/drm_drv.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
drivers/gpu/drm/drm_drv.c +1 -6
@@ -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); }
Signature · 6 lines
2.3.7 _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/3] drm: simplify master cleanup
Quoted 8 lines · 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 onmaster->uniqueand drop the redundant reset-code.Signed-off-by David Herrmann <[email protected]>
-Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/3] drm: simplify master cleanup
Quoted 10 lines · Chris Wilson
Quoted 8 lines · 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 onmaster->uniqueand drop the redundant reset-code.Signed-off-by David Herrmann <[email protected]>Reviewed-by Chris Wilson <[email protected]>
Both kernel and igt patches applied, thanks.
-Daniel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 4 lines · 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 <[email protected]>
-Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel