From patchwork Tue Jul 16 07:12:01 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 2827968 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id E25819F7D6 for ; Tue, 16 Jul 2013 07:28:28 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id CAE1D20177 for ; Tue, 16 Jul 2013 07:28:27 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id A97CE20176 for ; Tue, 16 Jul 2013 07:28:26 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 649F2E64CB for ; Tue, 16 Jul 2013 00:28:26 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-ea0-f172.google.com (mail-ea0-f172.google.com [209.85.215.172]) by gabe.freedesktop.org (Postfix) with ESMTP id DB4EBE6B02 for ; Tue, 16 Jul 2013 00:17:10 -0700 (PDT) Received: by mail-ea0-f172.google.com with SMTP id q10so160603eaj.3 for ; Tue, 16 Jul 2013 00:17:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=from:to:cc:subject:date:message-id:x-mailer:in-reply-to:references; bh=fH+i3V32xYq2/fMBPNnasN/OJ7jxs8ikEUIsXKphXvA=; b=iPzv7SJPog0qb8Xqx3YacGvxx1rSaBbH78safB35j8z88nQyFr1OLAW12jnlsHPpMA aIPiA8DsshhBIoJVSd44027io3/1GzC/ts6qrgwQ5OyxfxPeBLQ/MhuKLB3K26douQQA 8nMEWoQX2nSQNZAPVPWjffJfY8volpXhBpSOQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=from:to:cc:subject:date:message-id:x-mailer:in-reply-to:references :x-gm-message-state; bh=fH+i3V32xYq2/fMBPNnasN/OJ7jxs8ikEUIsXKphXvA=; b=T8YJh3zdL6WP9duxwptmOpUy4s5rWCv1lgE4lyb+J5lqzHIRsgdaTdd1+JRR/WJQQ4 nJ4O15MOK/TJrU09UkjyCy6slXyLL6z2lj+e9cwkdrsf8OcTOX1nENukOdiD2gx6ZOk2 lK0WWH8vdxdU2JjR5rc6PYUCNGdJsns7eMNighzLJvGwN9Kx4h2aXcP2cym/RauBjzAd R96TRnbixEtSmZs+jFwLCp5U1zi5S35iFwSeEkhJ71XNmLxWVwLJsPuK2+fkSpYHuJTm dJwIA0txB+/YYb6R7jqb76/NWwCGehBeUR6nJAdPIFUSjEXew7qJeruKzdFxj/bRIhan +cmg== X-Received: by 10.14.115.9 with SMTP id d9mr147030eeh.87.1373959030265; Tue, 16 Jul 2013 00:17:10 -0700 (PDT) Received: from aaron.ffwll.local (178-83-130-250.dynamic.hispeed.ch. [178.83.130.250]) by mx.google.com with ESMTPSA id n5sm283897eed.9.2013.07.16.00.17.08 for (version=TLSv1.2 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 16 Jul 2013 00:17:09 -0700 (PDT) From: Daniel Vetter To: DRI Development Subject: [PATCH 10/20] drm/gem: fix up flink name create race Date: Tue, 16 Jul 2013 09:12:01 +0200 Message-Id: <1373958731-4132-11-git-send-email-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 1.8.3.2 In-Reply-To: <1373958731-4132-1-git-send-email-daniel.vetter@ffwll.ch> References: <1373958731-4132-1-git-send-email-daniel.vetter@ffwll.ch> X-Gm-Message-State: ALoCoQm3+4jh4na7dZKYee9OzqHKC/xfRVKZ/+EyRfar3vk5SFFqHCd0kWHncBU2H36ZaLpmvSmL Cc: Daniel Vetter X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org Errors-To: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,RP_MATCHES_RCVD,T_DKIM_INVALID,UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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. v2: Fix up the error path handling in handle_create and make it more robust by simply calling object_handle_unreference. v3: Fix up the handle_unreference logic bug - atomic_dec_and_test retursn 1 for 0. Oops. Cc: Inki Dae Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_gem.c | 34 ++++++++++++++++++++------------- drivers/gpu/drm/drm_info.c | 2 +- drivers/gpu/drm/exynos/exynos_drm_gem.c | 2 +- include/drm/drmP.h | 12 ++++++++++-- 4 files changed, 33 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index b07519e..14c70b5 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -140,7 +140,7 @@ int drm_gem_object_init(struct drm_device *dev, return PTR_ERR(obj->filp); kref_init(&obj->refcount); - atomic_set(&obj->handle_count, 0); + obj->handle_count = 0; obj->size = size; return 0; @@ -161,7 +161,7 @@ int drm_gem_private_object_init(struct drm_device *dev, obj->filp = NULL; kref_init(&obj->refcount); - atomic_set(&obj->handle_count, 0); + obj->handle_count = 0; obj->size = size; return 0; @@ -227,11 +227,9 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj) struct drm_device *dev = obj->dev; /* Remove any name for this object */ - spin_lock(&dev->object_name_lock); if (obj->name) { idr_remove(&dev->object_name_idr, obj->name); obj->name = 0; - spin_unlock(&dev->object_name_lock); /* * The object name held a reference to this object, drop * that now. @@ -239,15 +237,13 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj) * This cannot be the last reference, since the handle holds one too. */ kref_put(&obj->refcount, drm_gem_object_ref_bug); - } else - spin_unlock(&dev->object_name_lock); - + } } void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) { - if (WARN_ON(atomic_read(&obj->handle_count) == 0)) + if (WARN_ON(obj->handle_count == 0)) return; /* @@ -256,8 +252,11 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) * checked for a name */ - if (atomic_dec_and_test(&obj->handle_count)) + spin_lock(&obj->dev->object_name_lock); + if (--obj->handle_count == 0) drm_gem_object_handle_free(obj); + spin_unlock(&obj->dev->object_name_lock); + drm_gem_object_unreference_unlocked(obj); } @@ -321,18 +320,21 @@ drm_gem_handle_create(struct drm_file *file_priv, * allocation under our spinlock. */ idr_preload(GFP_KERNEL); + spin_lock(&dev->object_name_lock); spin_lock(&file_priv->table_lock); ret = idr_alloc(&file_priv->object_idr, obj, 1, 0, GFP_NOWAIT); - + drm_gem_object_reference(obj); + obj->handle_count++; spin_unlock(&file_priv->table_lock); + spin_unlock(&dev->object_name_lock); idr_preload_end(); - if (ret < 0) + if (ret < 0) { + drm_gem_object_handle_unreference_unlocked(obj); return ret; + } *handlep = ret; - drm_gem_object_reference(obj); - atomic_inc(&obj->handle_count); if (dev->driver->gem_open_object) { ret = dev->driver->gem_open_object(obj, file_priv); @@ -499,6 +501,12 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data, idr_preload(GFP_KERNEL); spin_lock(&dev->object_name_lock); + /* prevent races with concurrent gem_close. */ + if (obj->handle_count == 0) { + ret = -ENOENT; + goto err; + } + if (!obj->name) { ret = idr_alloc(&dev->object_name_idr, obj, 1, 0, GFP_NOWAIT); if (ret < 0) diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c index d4b20ce..f4b348c 100644 --- a/drivers/gpu/drm/drm_info.c +++ b/drivers/gpu/drm/drm_info.c @@ -207,7 +207,7 @@ static int drm_gem_one_name_info(int id, void *ptr, void *data) seq_printf(m, "%6d %8zd %7d %8d\n", obj->name, obj->size, - atomic_read(&obj->handle_count), + obj->handle_count, atomic_read(&obj->refcount.refcount)); return 0; } diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c index 24c22a8..16963ca 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -135,7 +135,7 @@ void exynos_drm_gem_destroy(struct exynos_drm_gem_obj *exynos_gem_obj) obj = &exynos_gem_obj->base; buf = exynos_gem_obj->buffer; - DRM_DEBUG_KMS("handle count = %d\n", atomic_read(&obj->handle_count)); + DRM_DEBUG_KMS("handle count = %d\n", obj->handle_count); /* * do not release memory region from exporter. diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 2fb83b4..25da8e0 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -634,8 +634,16 @@ struct drm_gem_object { /** Reference count of this object */ struct kref refcount; - /** Handle count of this object. Each handle also holds a reference */ - atomic_t handle_count; /* number of handles on this object */ + /** + * handle_count - gem file_priv handle count of this object + * + * Each handle also holds a reference. Note that when the handle_count + * drops to 0 any global names (e.g. the id in the flink namespace) will + * be cleared. + * + * Protected by dev->object_name_lock. + * */ + unsigned handle_count; /** Related drm device */ struct drm_device *dev;