| Message ID | 1448462343-2072-2-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Do you need to take the mutex around other event pullers as well? So that no such process thinks it has pulled all events and then suddenly an event reappears? I think there was some event pulling code in one of the drivers, but I might be wrong. The close() code should be safe against this. /Thomas On 11/25/2015 03:39 PM, Chris Wilson wrote: > The previous patch reintroduced a race condition whereby a failure in > one reader may allow a second reader to see out-of-order events. > Introduce a mutex to serialise readers so that an event is completed in > its entirety before another reader may process an event. The two readers > may race against each other, but the events each retrieves are in the > correct order. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Thomas Hellstrom <thellstrom@vmware.com> > Cc: Takashi Iwai <tiwai@suse.de> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_fops.c | 18 +++++++++++++----- > include/drm/drmP.h | 2 ++ > 2 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c > index eb8702d39e7d..81df9ae95e2e 100644 > --- a/drivers/gpu/drm/drm_fops.c > +++ b/drivers/gpu/drm/drm_fops.c > @@ -172,6 +172,8 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor) > init_waitqueue_head(&priv->event_wait); > priv->event_space = 4096; /* set aside 4k for event buffer */ > > + mutex_init(&priv->event_read_lock); > + > if (drm_core_check_feature(dev, DRIVER_GEM)) > drm_gem_open(dev, priv); > > @@ -483,11 +485,15 @@ ssize_t drm_read(struct file *filp, char __user *buffer, > { > struct drm_file *file_priv = filp->private_data; > struct drm_device *dev = file_priv->minor->dev; > - ssize_t ret = 0; > + ssize_t ret; > > if (!access_ok(VERIFY_WRITE, buffer, count)) > return -EFAULT; > > + ret = mutex_lock_interruptible(&file_priv->event_read_lock); > + if (ret) > + return ret; > + > for (;;) { > struct drm_pending_event *e = NULL; > > @@ -509,12 +515,13 @@ ssize_t drm_read(struct file *filp, char __user *buffer, > break; > } > > + mutex_unlock(&file_priv->event_read_lock); > ret = wait_event_interruptible(file_priv->event_wait, > !list_empty(&file_priv->event_list)); > - if (ret < 0) > - break; > - > - ret = 0; > + if (ret >= 0) > + ret = mutex_lock_interruptible(&file_priv->event_read_lock); > + if (ret) > + return ret; > } else { > unsigned length = e->event->length; > > @@ -537,6 +544,7 @@ put_back_event: > e->destroy(e); > } > } > + mutex_unlock(&file_priv->event_read_lock); > > return ret; > } > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 30d4a5a495e2..8e1df1f7057c 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -344,6 +344,8 @@ struct drm_file { > struct list_head event_list; > int event_space; > > + struct mutex event_read_lock; > + > struct drm_prime_file_private prime; > }; >
On Wed, Nov 25, 2015 at 03:44:04PM +0100, Thomas Hellstrom wrote: > Do you need to take the mutex around other event pullers as well? We would. I checked in drm/*.c for other users, but not the drivers. A quick git grep doesn't show any likely candidates, they appear to be private event lists. > So that no such process thinks it has pulled all events and then > suddenly an event reappears? A short read just implies that the kernel returned all the events it has. That doesn't imply any new ones haven't manifested in the time it takes you to see the new events. (You either call read again until it EAGAINs, or go back to poll.) > I think there was some event pulling code in one of the drivers, but I > might be wrong. I hope not... > The close() code should be safe against this. I checked through drm_release and decided that since it cannot happen whilst drm_read() is active and so I didn't need to worry about having to break the lock or stop the read. Anything else of concern? -Chris
On 11/25/2015 03:56 PM, Chris Wilson wrote: > On Wed, Nov 25, 2015 at 03:44:04PM +0100, Thomas Hellstrom wrote: >> Do you need to take the mutex around other event pullers as well? > We would. I checked in drm/*.c for other users, but not the drivers. > A quick git grep doesn't show any likely candidates, they appear to be > private event lists. > > Indeed. I was confused by some exynos code I didn't look at too carefully. Also vmwgfx has some code to pull events, but it is not called until release time so it can't race. So for the series: Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com> Thanks for fixing this. /Thomas
On Thu, Nov 26, 2015 at 12:19:43PM +0100, Thomas Hellstrom wrote: > On 11/25/2015 03:56 PM, Chris Wilson wrote: > > On Wed, Nov 25, 2015 at 03:44:04PM +0100, Thomas Hellstrom wrote: > >> Do you need to take the mutex around other event pullers as well? > > We would. I checked in drm/*.c for other users, but not the drivers. > > A quick git grep doesn't show any likely candidates, they appear to be > > private event lists. > > > > > > Indeed. I was confused by some exynos code I didn't look at too > carefully. Also vmwgfx has some code to pull events, but it is not > called until release time so it can't race. > > So for the series: > Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com> Thanks for patches&review, applied to drm-misc. -Daniel
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index eb8702d39e7d..81df9ae95e2e 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -172,6 +172,8 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor) init_waitqueue_head(&priv->event_wait); priv->event_space = 4096; /* set aside 4k for event buffer */ + mutex_init(&priv->event_read_lock); + if (drm_core_check_feature(dev, DRIVER_GEM)) drm_gem_open(dev, priv); @@ -483,11 +485,15 @@ ssize_t drm_read(struct file *filp, char __user *buffer, { struct drm_file *file_priv = filp->private_data; struct drm_device *dev = file_priv->minor->dev; - ssize_t ret = 0; + ssize_t ret; if (!access_ok(VERIFY_WRITE, buffer, count)) return -EFAULT; + ret = mutex_lock_interruptible(&file_priv->event_read_lock); + if (ret) + return ret; + for (;;) { struct drm_pending_event *e = NULL; @@ -509,12 +515,13 @@ ssize_t drm_read(struct file *filp, char __user *buffer, break; } + mutex_unlock(&file_priv->event_read_lock); ret = wait_event_interruptible(file_priv->event_wait, !list_empty(&file_priv->event_list)); - if (ret < 0) - break; - - ret = 0; + if (ret >= 0) + ret = mutex_lock_interruptible(&file_priv->event_read_lock); + if (ret) + return ret; } else { unsigned length = e->event->length; @@ -537,6 +544,7 @@ put_back_event: e->destroy(e); } } + mutex_unlock(&file_priv->event_read_lock); return ret; } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 30d4a5a495e2..8e1df1f7057c 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -344,6 +344,8 @@ struct drm_file { struct list_head event_list; int event_space; + struct mutex event_read_lock; + struct drm_prime_file_private prime; };
The previous patch reintroduced a race condition whereby a failure in one reader may allow a second reader to see out-of-order events. Introduce a mutex to serialise readers so that an event is completed in its entirety before another reader may process an event. The two readers may race against each other, but the events each retrieves are in the correct order. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Thomas Hellstrom <thellstrom@vmware.com> Cc: Takashi Iwai <tiwai@suse.de> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_fops.c | 18 +++++++++++++----- include/drm/drmP.h | 2 ++ 2 files changed, 15 insertions(+), 5 deletions(-)