Re: [PATCH 18/20] drm/gem: completely close gem_open vs. gem_close races

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jul 16, 2013 at 09:12:09AM +0200, Daniel Vetter wrote:
> 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().
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

The gem_flink_race/flink_name subtest exercise this race here. Like
explained in the commit message strictly we don't need to care here, but
having the same logic for dma-buf and flink names is imo worthwile.

But now I've spotted a little bug in the the dma-buf import code, so gotta
write some dma-buf multithreaded testcases for that race now ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch