Re: [PATCH] drm/gem: fix up flink name create race

From: Daniel Vetter <daniel.vetter@ffwll.ch>
To: DRI Development <dri-devel@lists.freedesktop.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Subject: Re: [PATCH] drm/gem: fix up flink name create race
Date: Wed, 24 Jul 2013 11:02:02 +0200	[thread overview]
Message-ID: <CAKMK7uF6VxbEfAqWzqXKST2kDha_LtKtO0sOaAWu4cELkxooxg@mail.gmail.com> (raw)
In-Reply-To: <1374645842-18538-1-git-send-email-daniel.vetter@ffwll.ch>

On Wed, Jul 24, 2013 at 8:04 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> This is the 2nd attempt, I've always been a bit dissatisified with the
> tricky nature of the first one:
>
> http://lists.freedesktop.org/archives/dri-devel/2012-July/025451.html
>
> The issue is that the flink ioctl can race with calling gem_close on
> the last gem handle. In that case we'll end up with a zero handle
> count, but an flink name (and it's corresponding reference). Which
> results in a neat space leak.
>
> In my first attempt I've solved this by rechecking the handle count.
> But fundamentally the issue is that ->handle_count isn't your usual
> refcount - it can be resurrected from 0 among other things.
>
> For those special beasts atomic_t often suggest way more ordering that
> it actually guarantees. To prevent being tricked by those hairy
> semantics take the easy way out and simply protect the handle with the
> existing dev->object_name_lock.
>
> With that change implemented it's dead easy to fix the flink vs. gem
> close reace: When we try to create the name we simply have to check
> whether there's still officially a gem handle around and if not refuse
> to create the flink name. Since the handle count decrement and flink
> name destruction is now also protected by that lock the reace is gone
> and we can't ever leak the flink reference again.
>
> Outside of the drm core only the exynos driver looks at the handle
> count, and tbh I have no idea why (it's just for debug dmesg output
> luckily).
>
> I've considered inlining the drm_gem_object_handle_free, but I plan to
> add more name-like things (like the exported dma_buf) to this scheme,
> so it's clearer to leave the handle freeing in its own function.
>
> This is exercised by the new gem_flink_race i-g-t testcase, which on
> my snb leaks gem objects at a rate of roughly 1k objects/s.

That's actually incorrect since the leak I've found is just a race in
the drm/i915 object tracking. So I need to go back to the drawing
board and figure out which are the ghosts and which the dragons here.

I've turned that testcase into an exercise for "drm/gem: completely
close gem_open vs. gem_close races", but that race only results in
userspace seeing different flink names for the same object. And that
only happens if userspace is racy already.

For this patch here I still think there's an issue, but I seriously
need to restart my brain first and flush out the bogons with some
coffee before I try again ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch