From patchwork Wed Nov 25 14:39:02 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chris Wilson X-Patchwork-Id: 7699851 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 7B310BF90C for ; Wed, 25 Nov 2015 14:39:37 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id A5472208F1 for ; Wed, 25 Nov 2015 14:39:36 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id BEE5B208BC for ; Wed, 25 Nov 2015 14:39:35 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 182C26EAC9; Wed, 25 Nov 2015 06:39:35 -0800 (PST) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from fireflyinternet.com (mail.fireflyinternet.com [87.106.93.118]) by gabe.freedesktop.org (Postfix) with ESMTP id 3DCE86E6AA for ; Wed, 25 Nov 2015 06:39:31 -0800 (PST) X-Default-Received-SPF: pass (skip=forwardok (res=PASS)) x-ip-name=78.156.65.138; Received: from haswell.alporthouse.com (unverified [78.156.65.138]) by fireflyinternet.com (Firefly Internet (M1)) with ESMTP id 48797124-1500048 for multiple; Wed, 25 Nov 2015 14:39:20 +0000 Received: by haswell.alporthouse.com (sSMTP sendmail emulation); Wed, 25 Nov 2015 14:39:05 +0000 From: Chris Wilson To: dri-devel@lists.freedesktop.org Subject: [PATCH 1/2] drm: Drop dev->event_lock spinlock around faulting copy_to_user() Date: Wed, 25 Nov 2015 14:39:02 +0000 Message-Id: <1448462343-2072-1-git-send-email-chris@chris-wilson.co.uk> X-Mailer: git-send-email 2.6.2 In-Reply-To: <5654CD76.8000107@vmware.com> References: <5654CD76.8000107@vmware.com> X-Originating-IP: 78.156.65.138 X-Country: code=GB country="United Kingdom" ip=78.156.65.138 Cc: Daniel Vetter , Thomas Hellstrom X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, 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 In commit cdd1cf799bd24ac0a4184549601ae302267301c5 Author: Chris Wilson Date: Thu Dec 4 21:03:25 2014 +0000 drm: Make drm_read() more robust against multithreaded races I fixed the races by serialising the use of the event by extending the dev->event_lock. However, as Thomas pointed out, the copy_to_user() may fault (even the __copy_to_user_inatomic() variant used here) and calling into the driver backend with the spinlock held is bad news. Therefore we have to drop the spinlock before the copy, but that exposes us to the old race whereby a second reader could see an out-of-order event (as the first reader may claim the first request but fail to copy it back to userspace and so on returning it to the event list it will be behind the current event being copied by the second reader). Reported-by: Thomas Hellstrom Signed-off-by: Chris Wilson Cc: Thomas Hellstrom Cc: Takashi Iwai Cc: Daniel Vetter --- drivers/gpu/drm/drm_fops.c | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index c59ce4d0ef75..eb8702d39e7d 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -488,9 +488,19 @@ ssize_t drm_read(struct file *filp, char __user *buffer, if (!access_ok(VERIFY_WRITE, buffer, count)) return -EFAULT; - spin_lock_irq(&dev->event_lock); for (;;) { - if (list_empty(&file_priv->event_list)) { + struct drm_pending_event *e = NULL; + + spin_lock_irq(&dev->event_lock); + if (!list_empty(&file_priv->event_list)) { + e = list_first_entry(&file_priv->event_list, + struct drm_pending_event, link); + file_priv->event_space += e->event->length; + list_del(&e->link); + } + spin_unlock_irq(&dev->event_lock); + + if (e == NULL) { if (ret) break; @@ -499,36 +509,34 @@ ssize_t drm_read(struct file *filp, char __user *buffer, break; } - spin_unlock_irq(&dev->event_lock); ret = wait_event_interruptible(file_priv->event_wait, !list_empty(&file_priv->event_list)); - spin_lock_irq(&dev->event_lock); if (ret < 0) break; ret = 0; } else { - struct drm_pending_event *e; - - e = list_first_entry(&file_priv->event_list, - struct drm_pending_event, link); - if (e->event->length + ret > count) + unsigned length = e->event->length; + + if (length > count - ret) { +put_back_event: + spin_lock_irq(&dev->event_lock); + file_priv->event_space -= length; + list_add(&e->link, &file_priv->event_list); + spin_unlock_irq(&dev->event_lock); break; + } - if (__copy_to_user_inatomic(buffer + ret, - e->event, e->event->length)) { + if (copy_to_user(buffer + ret, e->event, length)) { if (ret == 0) ret = -EFAULT; - break; + goto put_back_event; } - file_priv->event_space += e->event->length; - ret += e->event->length; - list_del(&e->link); + ret += length; e->destroy(e); } } - spin_unlock_irq(&dev->event_lock); return ret; }