[PATCH] drm: Limit individual events to 1 KiB

[PATCH] drm: Limit individual events to 1 KiB

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Ever since support for vblank handling and event processing was added to
libdrm (v2.4.16), events were read in 1 KiB chunks in drmHandleEvent().
In addition, the implementation of drm_read() will not unqueue events if
they can't be successfully passed to userspace. The result is that it's
possible for a driver to send applications into a busy loop by creating
events larger than 1 KiB.

There is a limit to the event space that each DRM device can carry, but
it is set to 4 KiB.

While no driver currently produces events larger than 1 KiB, and it's
fairly unlikely that anyone ever will, it's best to err on the side of
caution and limit the size of individual events to 1 KiB. This ensures
that libdrm will never run into the busy loop. In order to allow for
the DRM device to queue multiple events, the event space limit is kept
at 4 KiB.

A complementary patch will be applied to libdrm to bump the read size
for events to 4 KiB to ensure that newer versions of libdrm running on
kernels without this fix will not run into the busy loop either.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_fops.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 48e106557c92..3792e4434319 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -582,6 +582,15 @@ EXPORT_SYMBOL(drm_poll);
  * This is the locked version of drm_event_reserve_init() for callers which
  * already hold dev->event_lock.
  *
+ * Note that libdrm has been reading events in chunks of 1 KiB since forever
+ * (see drmHandleEvent()) and up to and including v2.4.74. Unfortunately the
+ * drm_read() implementation won't return an error if an event doesn't fit
+ * into that 1 KiB and instead these events will simply be queued again. If
+ * a driver was to send out an event larger than 1 KiB, an application that
+ * polls the DRM file descriptor and calling drmHandleEvent() to process the
+ * events would go into a busy loop. This is caused by drm_poll() always
+ * return immediately, but drm_read() never being able to dequeue the event.
+ *
  * RETURNS:
  *
  * 0 on success or a negative error code on failure.
@@ -591,6 +600,15 @@ int drm_event_reserve_init_locked(struct drm_device *dev,
 				  struct drm_pending_event *p,
 				  struct drm_event *e)
 {
+	/*
+	 * Limit events to 1 KiB, which is the read size of libdrm (up to
+	 * to and including v2.4.74) when processing events, in order to
+	 * prevent any of the applications using libdrm from potentially
+	 * going into a busy loop.
+	 */
+	if (e->length > 1024)
+		return -EINVAL;
+
 	if (file_priv->event_space < e->length)
 		return -ENOMEM;
 
-- 
2.10.2

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

Re: [PATCH] drm: Limit individual events to 1 KiB

[Ville Syrjälä]

On Thu, Dec 15, 2016 at 12:48:34PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Ever since support for vblank handling and event processing was added to
> libdrm (v2.4.16), events were read in 1 KiB chunks in drmHandleEvent().
> In addition, the implementation of drm_read() will not unqueue events if
> they can't be successfully passed to userspace. The result is that it's
> possible for a driver to send applications into a busy loop by creating
> events larger than 1 KiB.
> 
> There is a limit to the event space that each DRM device can carry, but
> it is set to 4 KiB.
> 
> While no driver currently produces events larger than 1 KiB, and it's
> fairly unlikely that anyone ever will, it's best to err on the side of
> caution and limit the size of individual events to 1 KiB. This ensures
> that libdrm will never run into the busy loop. In order to allow for
> the DRM device to queue multiple events, the event space limit is kept
> at 4 KiB.
> 
> A complementary patch will be applied to libdrm to bump the read size
> for events to 4 KiB to ensure that newer versions of libdrm running on
> kernels without this fix will not run into the busy loop either.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_fops.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 48e106557c92..3792e4434319 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -582,6 +582,15 @@ EXPORT_SYMBOL(drm_poll);
>   * This is the locked version of drm_event_reserve_init() for callers which
>   * already hold dev->event_lock.
>   *
> + * Note that libdrm has been reading events in chunks of 1 KiB since forever
> + * (see drmHandleEvent()) and up to and including v2.4.74. Unfortunately the
> + * drm_read() implementation won't return an error if an event doesn't fit
> + * into that 1 KiB and instead these events will simply be queued again. If
> + * a driver was to send out an event larger than 1 KiB, an application that
> + * polls the DRM file descriptor and calling drmHandleEvent() to process the
> + * events would go into a busy loop. This is caused by drm_poll() always
> + * return immediately, but drm_read() never being able to dequeue the event.
> + *
>   * RETURNS:
>   *
>   * 0 on success or a negative error code on failure.
> @@ -591,6 +600,15 @@ int drm_event_reserve_init_locked(struct drm_device *dev,
>  				  struct drm_pending_event *p,
>  				  struct drm_event *e)
>  {
> +	/*
> +	 * Limit events to 1 KiB, which is the read size of libdrm (up to
> +	 * to and including v2.4.74) when processing events, in order to
> +	 * prevent any of the applications using libdrm from potentially
> +	 * going into a busy loop.
> +	 */
> +	if (e->length > 1024)
> +		return -EINVAL;

Are we really expecting events that large? A much lower limit,
accompanied by a WARN/BUG, is what I'd go for.

> +
>  	if (file_priv->event_space < e->length)
>  		return -ENOMEM;
>  
> -- 
> 2.10.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Limit individual events to 1 KiB

[Jani Nikula]

On Thu, 15 Dec 2016, Thierry Reding <thierry.reding@gmail.com> wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> Ever since support for vblank handling and event processing was added to
> libdrm (v2.4.16), events were read in 1 KiB chunks in drmHandleEvent().
> In addition, the implementation of drm_read() will not unqueue events if
> they can't be successfully passed to userspace. The result is that it's
> possible for a driver to send applications into a busy loop by creating
> events larger than 1 KiB.
>
> There is a limit to the event space that each DRM device can carry, but
> it is set to 4 KiB.
>
> While no driver currently produces events larger than 1 KiB, and it's
> fairly unlikely that anyone ever will, it's best to err on the side of
> caution and limit the size of individual events to 1 KiB. This ensures
> that libdrm will never run into the busy loop. In order to allow for
> the DRM device to queue multiple events, the event space limit is kept
> at 4 KiB.
>
> A complementary patch will be applied to libdrm to bump the read size
> for events to 4 KiB to ensure that newer versions of libdrm running on
> kernels without this fix will not run into the busy loop either.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_fops.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 48e106557c92..3792e4434319 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -582,6 +582,15 @@ EXPORT_SYMBOL(drm_poll);
>   * This is the locked version of drm_event_reserve_init() for callers which
>   * already hold dev->event_lock.
>   *
> + * Note that libdrm has been reading events in chunks of 1 KiB since forever
> + * (see drmHandleEvent()) and up to and including v2.4.74. Unfortunately the

Maybe it's a case of post-lunch catatonia, but I actually read that a
few times, and thought it made no sense, until I realized the version
number refers to libdrm, not kernel... Would it be too verbose to use
"libdrm v2.4.74" explicitly here and below?

BR,
Jani.

> + * drm_read() implementation won't return an error if an event doesn't fit
> + * into that 1 KiB and instead these events will simply be queued again. If
> + * a driver was to send out an event larger than 1 KiB, an application that
> + * polls the DRM file descriptor and calling drmHandleEvent() to process the
> + * events would go into a busy loop. This is caused by drm_poll() always
> + * return immediately, but drm_read() never being able to dequeue the event.
> + *
>   * RETURNS:
>   *
>   * 0 on success or a negative error code on failure.
> @@ -591,6 +600,15 @@ int drm_event_reserve_init_locked(struct drm_device *dev,
>  				  struct drm_pending_event *p,
>  				  struct drm_event *e)
>  {
> +	/*
> +	 * Limit events to 1 KiB, which is the read size of libdrm (up to
> +	 * to and including v2.4.74) when processing events, in order to
> +	 * prevent any of the applications using libdrm from potentially
> +	 * going into a busy loop.
> +	 */
> +	if (e->length > 1024)
> +		return -EINVAL;
> +
>  	if (file_priv->event_space < e->length)
>  		return -ENOMEM;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel