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 at chris-wilson.co.uk>
> Cc: Thomas Hellstrom <thellstrom at vmware.com>
> Cc: Takashi Iwai <tiwai at suse.de>
> Cc: Daniel Vetter <daniel.vetter at 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;
> };
>
