[PATCH 2/3] drm: simplify authentication management

[David Herrmann]

Hi

On Mon, May 4, 2015 at 4:46 PM, Chris Wilson <chris at 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 at 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