Re: [PATCH 1/2] drm: Fix memory leak at error path of drm_read()

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Daniel Vetter <daniel@ffwll.ch>, Takashi Iwai <tiwai@suse.de>,
	dri-devel@lists.freedesktop.org,
	Maarten Lankhorst <maarten.lankhorst@canonical.com>
Subject: Re: [PATCH 1/2] drm: Fix memory leak at error path of drm_read()
Date: Thu, 4 Dec 2014 18:25:08 +0100	[thread overview]
Message-ID: <20141204172508.GG20350@phenom.ffwll.local> (raw)
In-Reply-To: <20141204163115.GF13586@nuc-i3427.alporthouse.com>

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 <tiwai@suse.de>
> > > > ---
> > > >  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.

Well we essentially send out datagrams instead of a bytestream. If we look
at recvmsg and friends then discarding the additional bytes is something
that's already being done. So I'm not terribly concerned about that
either. It's a bit non-pretty that we use read and not reicvmsg but since
we use read already not something we can ever fix.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel