linux-kernel - Re: [PATCH v2 2/2] drm/fence: allow fence waiting to be interrupted by userspace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <cb572389-f1ae-65a7-7dd3-f314d7a9f744@linux.intel.com>
Date:	Thu, 18 Aug 2016 10:28:06 +0200
From:	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
To:	Gustavo Padovan <gustavo@...ovan.org>,
	dri-devel@...ts.freedesktop.org
Cc:	linux-kernel@...r.kernel.org, Daniel Stone <daniels@...labora.com>,
	Daniel Vetter <daniel.vetter@...ll.ch>,
	Rob Clark <robdclark@...il.com>,
	Greg Hackmann <ghackmann@...gle.com>,
	John Harrison <John.C.Harrison@...el.com>,
	laurent.pinchart@...asonboard.com, seanpaul@...gle.com,
	marcheu@...gle.com, m.chehab@...sung.com,
	Sumit Semwal <sumit.semwal@...aro.org>,
	Gustavo Padovan <gustavo.padovan@...labora.co.uk>,
	Daniel Vetter <daniel@...ll.ch>
Subject: Re: [PATCH v2 2/2] drm/fence: allow fence waiting to be interrupted
 by userspace

Op 16-08-16 om 01:05 schreef Gustavo Padovan:
> From: Gustavo Padovan <gustavo.padovan@...labora.co.uk>
>
> If userspace is running an synchronously atomic commit and interrupts the
> atomic operation during fence_wait() it will hang until the timer expires,
> so here we change the wait to be interruptible so it stop immediately when
> userspace wants to quit.
>
> Also adds the necessary error checking for fence_wait().
>
> v2: Comment by Daniel Vetter
> 	- Add error checking for fence_wait()
>
> v3: Rebase on top of new atomic noblocking support
>
> v4: Comment by Maarten Lankhorst
> 	- remove 'swapped' bitfield as it was duplicating information
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@...labora.co.uk>
I think it would make sense to squash this patch with 1/2.
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 33 ++++++++++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index bf1bc43..292d15b 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1022,20 +1022,37 @@ int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
>  {
>  	struct drm_plane *plane;
>  	struct drm_plane_state *plane_state;
> +	struct fence *fence;
>  	int i, ret;
>  
>  	for_each_plane_in_state(state, plane, plane_state, i) {
> -		if (!plane->state->fence)
> +		if (!plane->state->fence && !plane_state->fence)
>  			continue;
Move this check to below, or put if (!intr) plane_state = plane->state on top, and only use plane_state.

Daniel, I keep thinking more and more that a for_each_XXX_(old,new,both)_state would be useful,
could we add those perhaps?
> -		WARN_ON(!plane->state->fb);
> +		/*
> +		 * If the caller asks for an interruptible wait it means
> +		 * that the state were not swapped yet and the operation
> +		 * can still be interrupted by userspace, so we need
> +		 * to look to plane_state instead.
> +		 */
> +		if (intr) {
> +			fence = plane_state->fence;
> +			WARN_ON(!plane_state->fb);
> +		} else {
> +			fence = plane->state->fence;
> +			WARN_ON(!plane->state->fb);
> +		}
^ if (!fence) continue;
> -		ret = fence_wait(plane->state->fence, intr);
> +		ret = fence_wait(fence, intr);
>  		if (ret)
>  			return ret;
>  
> -		fence_put(plane->state->fence);
> -		plane->state->fence = NULL;
> +		fence_put(fence);
> +
> +		if (intr)
> +			plane_state->fence = NULL;
> +		else
> +			plane->state->fence = NULL;
>  	}
>  
>  	return 0;
> @@ -1244,6 +1261,12 @@ int drm_atomic_helper_commit(struct drm_device *dev,
>  	if (ret)
>  		return ret;
>  
> +	if (!nonblock) {
> +		ret = drm_atomic_helper_wait_for_fences(dev, state, true);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	/*
>  	 * This is the point of no return - everything below never fails except
>  	 * when the hw goes bonghits. Which means we can commit the new state on