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@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.
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? -Chris