From patchwork Thu Dec 4 16:38:57 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chris Wilson X-Patchwork-Id: 5439031 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 0D5FE9F319 for ; Thu, 4 Dec 2014 16:39:06 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 2324D2012D for ; Thu, 4 Dec 2014 16:39:05 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 3024D20103 for ; Thu, 4 Dec 2014 16:39:04 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 712E56EA4E; Thu, 4 Dec 2014 08:39:03 -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 116BA6EAEB for ; Thu, 4 Dec 2014 08:39:01 -0800 (PST) X-Default-Received-SPF: pass (skip=forwardok (res=PASS)) x-ip-name=78.156.65.138; Received: from nuc-i3427.alporthouse.com (unverified [78.156.65.138]) by fireflyinternet.com (Firefly Internet (M1)) with ESMTP id 33696803-1500048 for multiple; Thu, 04 Dec 2014 16:39:24 +0000 Received: by nuc-i3427.alporthouse.com (sSMTP sendmail emulation); Thu, 04 Dec 2014 16:38:57 +0000 Date: Thu, 4 Dec 2014 16:38:57 +0000 From: Chris Wilson To: Daniel Vetter , Takashi Iwai , dri-devel@lists.freedesktop.org, Maarten Lankhorst Subject: Re: [PATCH 1/2] drm: Fix memory leak at error path of drm_read() Message-ID: <20141204163857.GG13586@nuc-i3427.alporthouse.com> Mail-Followup-To: Chris Wilson , Daniel Vetter , Takashi Iwai , dri-devel@lists.freedesktop.org, Maarten Lankhorst References: <1417690603-1595-1-git-send-email-tiwai@suse.de> <20141204115114.GB13586@nuc-i3427.alporthouse.com> <20141204122839.GE20350@phenom.ffwll.local> <20141204163115.GF13586@nuc-i3427.alporthouse.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20141204163115.GF13586@nuc-i3427.alporthouse.com> User-Agent: Mutt/1.5.21 (2010-09-15) 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: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_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 On Thu, Dec 04, 2014 at 04:31:15PM +0000, Chris Wilson wrote: > On Thu, Dec 04, 2014 at 01:28:39PM +0100, Daniel Vetter wrote: > > On Thu, Dec 04, 2014 at 11:51:14AM +0000, Chris Wilson wrote: > > > On Thu, Dec 04, 2014 at 11:56:42AM +0100, Takashi Iwai wrote: > > > > Signed-off-by: Takashi Iwai > > > > --- > > > > drivers/gpu/drm/drm_fops.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c > > > > index ed7bc68f7e87..a82dc28d54f3 100644 > > > > --- a/drivers/gpu/drm/drm_fops.c > > > > +++ b/drivers/gpu/drm/drm_fops.c > > > > @@ -525,6 +525,7 @@ ssize_t drm_read(struct file *filp, char __user *buffer, > > > > if (copy_to_user(buffer + total, > > > > e->event, e->event->length)) { > > > > total = -EFAULT; > > > > + e->destroy(e); > > > > > > We shouldn't just be throwing away the event here, but put the event > > > back at the front of the queue. Poses an interesting race issue. Seems > > > like we want to hold the spinlock until the copy is complete so that we > > > can fix up the failure correctly. > > > > I've read the manpage for read and it explicitly states that when you get > > an error it's undefined what happens to the read position. Since -EFAULT > > is really just a userspace bug I think we can happily drop the event on > > the floor, no reason to bend over in the kernel. > > Hmm. Actually the code is buggy is the provided buffer is too short for > the first event in O_NONBLOCK mode. Just inlining drm_dequeue_event is simple enough to fix the issue: which looks familar to the previous attempt to get O_NONBLOCK fixed. -Chris diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 91e1105..7af6676 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -478,43 +478,17 @@ int drm_release(struct inode *inode, struct file *filp) } EXPORT_SYMBOL(drm_release); -static bool -drm_dequeue_event(struct drm_file *file_priv, - size_t total, size_t max, struct drm_pending_event **out) -{ - struct drm_device *dev = file_priv->minor->dev; - struct drm_pending_event *e; - unsigned long flags; - bool ret = false; - - spin_lock_irqsave(&dev->event_lock, flags); - - *out = NULL; - if (list_empty(&file_priv->event_list)) - goto out; - e = list_first_entry(&file_priv->event_list, - struct drm_pending_event, link); - if (e->event->length + total > max) - goto out; - - file_priv->event_space += e->event->length; - list_del(&e->link); - *out = e; - ret = true; - -out: - spin_unlock_irqrestore(&dev->event_lock, flags); - return ret; -} - ssize_t drm_read(struct file *filp, char __user *buffer, size_t count, loff_t *offset) { struct drm_file *file_priv = filp->private_data; - struct drm_pending_event *e; - size_t total; + struct drm_device *dev = file_priv->minor->dev; + unsigned long flags; ssize_t ret; + if (!access_ok(VERIFY_WRITE, buffer, count)) + return -EFAULT; + if ((filp->f_flags & O_NONBLOCK) == 0) { ret = wait_event_interruptible(file_priv->event_wait, !list_empty(&file_priv->event_list)); @@ -522,19 +496,35 @@ ssize_t drm_read(struct file *filp, char __user *buffer, return ret; } - total = 0; - while (drm_dequeue_event(file_priv, total, count, &e)) { - if (copy_to_user(buffer + total, - e->event, e->event->length)) { - total = -EFAULT; - break; - } + spin_lock_irqsave(&dev->event_lock, flags); + if (list_empty(&file_priv->event_list)) { + ret = -EAGAIN; + } else { + ret = 0; + do { + struct drm_pending_event *e; + + e = list_first_entry(&file_priv->event_list, + struct drm_pending_event, link); + if (e->event->length + ret > count) + break; + + if (__copy_to_user_inatomic(buffer + ret, + e->event, e->event->length)) { + if (ret == 0) + ret = -EFAULT; + break; + } - total += e->event->length; - e->destroy(e); + file_priv->event_space += e->event->length; + ret += e->event->length; + list_del(&e->link); + e->destroy(e); + } while (!list_empty(&file_priv->event_list)); } + spin_unlock_irqrestore(&dev->event_lock, flags); - return total ?: -EAGAIN; + return ret; } EXPORT_SYMBOL(drm_read);