drm/gem: completely close gem_open vs. gem_close races
authorDaniel Vetter <daniel.vetter@ffwll.ch>
Wed, 14 Aug 2013 22:02:45 +0000 (00:02 +0200)
committerChanho Park <chanho61.park@samsung.com>
Fri, 21 Nov 2014 10:13:43 +0000 (19:13 +0900)
commit462da3c532f569d497d8f506f2d027c78e0dc638
treeb6e6470d64c166583c99b847d9c5c3c7dcd7155a
parentdf1db9bf3ff56007e063b26f4c4a4c1b478c192a
drm/gem: completely close gem_open vs. gem_close races

The gem flink name holds a reference onto the object itself, and this
self-reference would prevent an flink'ed object from every being
freed. To break that loop we remove the flink name when the last
userspace handle disappears, i.e. when obj->handle_count reaches 0.

Now in gem_open we drop the dev->object_name_lock between the flink
name lookup and actually adding the handle. This means a concurrent
gem_close of the last handle could result in the flink name getting
reaped right inbetween, i.e.

Thread 1 Thread 2
gem_open gem_close

flink -> obj lookup
handle_count drops to 0
remove flink name
create_handle
handle_count++

If someone now flinks this object again, we'll get a new flink name.

We can close this race by removing the lock dropping and making the
entire lookup+handle_create sequence atomic. Unfortunately to still be
able to share the handle_create logic this requires a
handle_create_tail function which drops the lock - we can't hold the
object_name_lock while calling into a driver's ->gem_open callback.

Note that for flink fixing this race isn't really important, since
racing gem_open against gem_close is clearly a userspace bug. And no
matter how the race ends, we won't leak any references.

But with dma-buf where the userspace dma-buf fd itself is refcounted
this is a valid sequence and hence we should fix it. Therefore this
patch here is just a warm-up exercise (and for consistency between
flink buffer sharing and dma-buf buffer sharing with self-imports).

Also note that this extension of the critical section in gem_open
protected by dev->object_name_lock only works because it's now a
mutex: A spinlock would conflict with the potential memory allocation
in idr_preload().

This is exercises by igt/gem_flink_race/flink_name.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
drivers/gpu/drm/drm_gem.c
include/drm/drmP.h