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 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.
>
> 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.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
