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)
committerJoonyoung Shim <jy0922.shim@samsung.com>
Wed, 13 Jan 2016 01:29:21 +0000 (10:29 +0900)
commit9d4d0666fba96370c54e0d704edc85467ac8094f
tree09aaa0e0f2595178688204229b807b04b66b024c
parent9348f9e7306e8fdb0b1c58d7392071fc8f3cdf4b
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>
[jy0922.shim: fix up fuzz to apply]
Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
Change-Id: I7fc3ffb1a77b2b5ca7e04a38c26ccd3a73b67f62
drivers/gpu/drm/drm_gem.c
include/drm/drmP.h