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

[Takashi Iwai]

At Thu, 4 Dec 2014 12:13:46 +0000,
Chris Wilson wrote:
> 
> On Thu, Dec 04, 2014 at 12:56:38PM +0100, Takashi Iwai wrote:
> > At Thu, 4 Dec 2014 11:51:14 +0000,
> > Chris Wilson wrote:
> > > 
> > > On Thu, Dec 04, 2014 at 11:56:42AM +0100, Takashi Iwai wrote:
> > > > Signed-off-by: Takashi Iwai <tiwai at 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.
> > 
> > Yeah, I thought of it while writing this, but as a starter, I tried
> > the simpler one.  (And I didn't realize your comment referring to the
> > already existing fix in kernel, sorry!)
> > 
> > The problem to hold the spinlock for the whole is that you can't it
> > with copy_to_user().  So it'd be a bit tricky.
> 
> We can use access_ok(VERFIY_WRITE) and then copy_to_user_inatomic().
> Doable with a bit of code rearrangement.

Yes, but this is really not necessarily atomic.  We can simply prepend
the event back in the error case, instead, too.

> > And, why not using event_wait.lock instead of the extra event_lock?
> > Then we can use wait_event_lock_*() variant that covers more race
> > between dequeuing and wait_event.
> 
> The tricky part would appear to be then protecting
> dev->vblank_event_list. That looks like it could be an RCU list instead?

Sounds interesting.


Takashi