Re: drm_read() to paged-out memory area

[Thomas Hellstrom <thellstrom@vmware.com>]

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