[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 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.

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