Re: drm_read() to paged-out memory area

Re: drm_read() to paged-out memory area

[Chris Wilson]

On Tue, Nov 24, 2015 at 09:49:58PM +0100, Thomas Hellstrom wrote:
> Hi, Chris,
> 
> With your new (well sort of) implementation of drm_read() it looks to me
> like a drm_read() to a paged out
> memory area will always fail with -EFAULT. From what I can tell, there's
> nothing there to generate a page-fault to get the destination paged back
> into memory?

True. Whoops.

If we take the first event (remove it from the list from a second reader
cannot see it), fail to copy it and replace it at the head, the second
reader may then get an out-of-order event.

First we need something like:

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index c59ce4d0ef75..b9ca549e0e99 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -509,14 +509,21 @@ ssize_t drm_read(struct file *filp, char __user *buffer,
                        ret = 0;
                } else {
                        struct drm_pending_event *e;
+                       int unwritten;
 
                        e = list_first_entry(&file_priv->event_list,
                                             struct drm_pending_event, link);
                        if (e->event->length + ret > count)
                                break;
 
-                       if (__copy_to_user_inatomic(buffer + ret,
-                                                   e->event, e->event->length)) {
+                       list_del(&e->link);
+
+                       spin_unlock_irq(&dev->event_lock);
+                       unwritten = copy_to_user(buffer + ret,
+                                                e->event, e->event->length);
+                       spin_lock_irq(&dev->event_lock);
+                       if (unwritten) {
+                               list_add(&e->link, &file_priv->event_list);
                                if (ret == 0)
                                        ret = -EFAULT;
                                break;
@@ -524,7 +531,6 @@ ssize_t drm_read(struct file *filp, char __user *buffer,
 
                        file_priv->event_space += e->event->length;
                        ret += e->event->length;
-                       list_del(&e->link);
                        e->destroy(e);
                }
        }


A task for tomorrow (along with a suitable testcase).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: drm_read() to paged-out memory area

[Thomas Hellstrom]

Hi!


On 11/24/2015 11:14 PM, Chris Wilson wrote:
> On Tue, Nov 24, 2015 at 09:49:58PM +0100, Thomas Hellstrom wrote:
>> Hi, Chris,
>>
>> With your new (well sort of) implementation of drm_read() it looks to me
>> like a drm_read() to a paged out
>> memory area will always fail with -EFAULT. From what I can tell, there's
>> nothing there to generate a page-fault to get the destination paged back
>> into memory?
> True. Whoops.
>
> If we take the first event (remove it from the list from a second reader
> cannot see it), fail to copy it and replace it at the head, the second
> reader may then get an out-of-order event.
>
> First we need something like:


A simple solution would be to add a new file_priv::read_mutex around all
functions that pull
stuff from the event queue, so that adding takes only the spinlock,
reading takes the mutex and the spinlock. There aren't that many such
places from what I can tell. We probably should keep the mutex held
around the wait for data during a blocking read, but make sure that any
locks are taken interruptibly so that we can signal the waiting process.

/Thomas


>
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index c59ce4d0ef75..b9ca549e0e99 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -509,14 +509,21 @@ ssize_t drm_read(struct file *filp, char __user *buffer,
>                         ret = 0;
>                 } else {
>                         struct drm_pending_event *e;
> +                       int unwritten;
>  
>                         e = list_first_entry(&file_priv->event_list,
>                                              struct drm_pending_event, link);
>                         if (e->event->length + ret > count)
>                                 break;
>  
> -                       if (__copy_to_user_inatomic(buffer + ret,
> -                                                   e->event, e->event->length)) {
> +                       list_del(&e->link);
> +
> +                       spin_unlock_irq(&dev->event_lock);
> +                       unwritten = copy_to_user(buffer + ret,
> +                                                e->event, e->event->length);
> +                       spin_lock_irq(&dev->event_lock);
> +                       if (unwritten) {
> +                               list_add(&e->link, &file_priv->event_list);
>                                 if (ret == 0)
>                                         ret = -EFAULT;
>                                 break;
> @@ -524,7 +531,6 @@ ssize_t drm_read(struct file *filp, char __user *buffer,
>  
>                         file_priv->event_space += e->event->length;
>                         ret += e->event->length;
> -                       list_del(&e->link);
>                         e->destroy(e);
>                 }
>         }
>
>
> A task for tomorrow (along with a suitable testcase).
> -Chris
>


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: drm_read() to paged-out memory area

[Chris Wilson]

On Tue, Nov 24, 2015 at 10:14:20PM +0000, Chris Wilson wrote:
> On Tue, Nov 24, 2015 at 09:49:58PM +0100, Thomas Hellstrom wrote:
> > Hi, Chris,
> > 
> > With your new (well sort of) implementation of drm_read() it looks to me
> > like a drm_read() to a paged out
> > memory area will always fail with -EFAULT. From what I can tell, there's
> > nothing there to generate a page-fault to get the destination paged back
> > into memory?
> 
> True. Whoops.

Ok, I've added a test case (igt/drm_read/fault-buffer) that forces a
pagefault here by using a dumb bo as the destination buffer. (At least
on i915, we do not do any prefaulting inside the mmap routine.)

That demonstrates that __copy_to_user_inatomic() itself does not disable
pagefaults (i.e. it does not call pagefault_disable) and hence why we
don't always immediately generate EFAULT.

That leaves us with the issue that the fault handler may still deadlock
on the dev->event_lock, so it still needs to be fixed.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm: Drop dev->event_lock spinlock around faulting copy_to_user()

[Chris Wilson]

In

commit cdd1cf799bd24ac0a4184549601ae302267301c5
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Dec 4 21:03:25 2014 +0000

    drm: Make drm_read() more robust against multithreaded races

I fixed the races by serialising the use of the event by extending the
dev->event_lock. However, as Thomas pointed out, the copy_to_user() may
fault (even the __copy_to_user_inatomic() variant used here) and calling
into the driver backend with the spinlock held is bad news. Therefore we
have to drop the spinlock before the copy, but that exposes us to the
old race whereby a second reader could see an out-of-order event (as the
first reader may claim the first request but fail to copy it back to
userspace and so on returning it to the event list it will be behind the
current event being copied by the second reader).

Reported-by: Thomas Hellstrom <thellstrom@vmware.com>
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 | 40 ++++++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index c59ce4d0ef75..eb8702d39e7d 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -488,9 +488,19 @@ ssize_t drm_read(struct file *filp, char __user *buffer,
 	if (!access_ok(VERIFY_WRITE, buffer, count))
 		return -EFAULT;
 
-	spin_lock_irq(&dev->event_lock);
 	for (;;) {
-		if (list_empty(&file_priv->event_list)) {
+		struct drm_pending_event *e = NULL;
+
+		spin_lock_irq(&dev->event_lock);
+		if (!list_empty(&file_priv->event_list)) {
+			e = list_first_entry(&file_priv->event_list,
+					struct drm_pending_event, link);
+			file_priv->event_space += e->event->length;
+			list_del(&e->link);
+		}
+		spin_unlock_irq(&dev->event_lock);
+
+		if (e == NULL) {
 			if (ret)
 				break;
 
@@ -499,36 +509,34 @@ ssize_t drm_read(struct file *filp, char __user *buffer,
 				break;
 			}
 
-			spin_unlock_irq(&dev->event_lock);
 			ret = wait_event_interruptible(file_priv->event_wait,
 						       !list_empty(&file_priv->event_list));
-			spin_lock_irq(&dev->event_lock);
 			if (ret < 0)
 				break;
 
 			ret = 0;
 		} else {
-			struct drm_pending_event *e;
-
-			e = list_first_entry(&file_priv->event_list,
-					     struct drm_pending_event, link);
-			if (e->event->length + ret > count)
+			unsigned length = e->event->length;
+
+			if (length > count - ret) {
+put_back_event:
+				spin_lock_irq(&dev->event_lock);
+				file_priv->event_space -= length;
+				list_add(&e->link, &file_priv->event_list);
+				spin_unlock_irq(&dev->event_lock);
 				break;
+			}
 
-			if (__copy_to_user_inatomic(buffer + ret,
-						    e->event, e->event->length)) {
+			if (copy_to_user(buffer + ret, e->event, length)) {
 				if (ret == 0)
 					ret = -EFAULT;
-				break;
+				goto put_back_event;
 			}
 
-			file_priv->event_space += e->event->length;
-			ret += e->event->length;
-			list_del(&e->link);
+			ret += length;
 			e->destroy(e);
 		}
 	}
-	spin_unlock_irq(&dev->event_lock);
 
 	return ret;
 }
-- 
2.6.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm: Serialise multiple event readers

[Chris Wilson]

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;
 };
 
-- 
2.6.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: Serialise multiple event readers

[Thomas Hellstrom]

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;
>  };
>  

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: Serialise multiple event readers

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: Serialise multiple event readers

[Thomas Hellstrom]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: Serialise multiple event readers

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel