[PATCH 1/2] drm/amdgpu: fix race in GEM VA map IOCTL

[PATCH 1/2] drm/amdgpu: fix race in GEM VA map IOCTL

[Christian König]

From: Christian König <christian.koenig@amd.com>

Somebody could try to free the bo_va between mapping and updating it.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 45 ++++++++++-----------------------
 1 file changed, 14 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 7a265d9..ec5b184 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -489,67 +489,50 @@ static int amdgpu_gem_va_check(void *param, struct amdgpu_bo *bo)
  *
  * @adev: amdgpu_device pointer
  * @bo_va: bo_va to update
+ * @list: validation list
+ * opration: map or unmap
  *
  * Update the bo_va directly after setting it's address. Errors are not
  * vital here, so they are not reported back to userspace.
  */
 static void amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
 				    struct amdgpu_bo_va *bo_va,
+				    struct list_head *list,
 				    uint32_t operation)
 {
-	struct ttm_validate_buffer tv, *entry;
-	struct amdgpu_bo_list_entry vm_pd;
-	struct ww_acquire_ctx ticket;
-	struct list_head list, duplicates;
-	int r;
-
-	INIT_LIST_HEAD(&list);
-	INIT_LIST_HEAD(&duplicates);
-
-	tv.bo = &bo_va->bo->tbo;
-	tv.shared = true;
-	list_add(&tv.head, &list);
-
-	amdgpu_vm_get_pd_bo(bo_va->vm, &list, &vm_pd);
-
-	/* Provide duplicates to avoid -EALREADY */
-	r = ttm_eu_reserve_buffers(&ticket, &list, true, &duplicates);
-	if (r)
-		goto error_print;
+	struct ttm_validate_buffer *entry;
+	int r = -ERESTARTSYS;
 
-	list_for_each_entry(entry, &list, head) {
+	list_for_each_entry(entry, list, head) {
 		struct amdgpu_bo *bo =
 			container_of(entry->bo, struct amdgpu_bo, tbo);
 
 		/* if anything is swapped out don't swap it in here,
 		   just abort and wait for the next CS */
 		if (!amdgpu_bo_gpu_accessible(bo))
-			goto error_unreserve;
+			goto error;
 
 		if (bo->shadow && !amdgpu_bo_gpu_accessible(bo->shadow))
-			goto error_unreserve;
+			goto error;
 	}
 
 	r = amdgpu_vm_validate_pt_bos(adev, bo_va->vm, amdgpu_gem_va_check,
 				      NULL);
 	if (r)
-		goto error_unreserve;
+		goto error;
 
 	r = amdgpu_vm_update_page_directory(adev, bo_va->vm);
 	if (r)
-		goto error_unreserve;
+		goto error;
 
 	r = amdgpu_vm_clear_freed(adev, bo_va->vm);
 	if (r)
-		goto error_unreserve;
+		goto error;
 
 	if (operation == AMDGPU_VA_OP_MAP)
 		r = amdgpu_vm_bo_update(adev, bo_va, false);
 
-error_unreserve:
-	ttm_eu_backoff_reservation(&ticket, &list);
-
-error_print:
+error:
 	if (r && r != -ERESTARTSYS)
 		DRM_ERROR("Couldn't update BO_VA (%d)\n", r);
 }
@@ -642,10 +625,10 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
 	default:
 		break;
 	}
-	ttm_eu_backoff_reservation(&ticket, &list);
 	if (!r && !(args->flags & AMDGPU_VM_DELAY_UPDATE) &&
 	    !amdgpu_vm_debug)
-		amdgpu_gem_va_update_vm(adev, bo_va, args->operation);
+		amdgpu_gem_va_update_vm(adev, bo_va, &list, args->operation);
+	ttm_eu_backoff_reservation(&ticket, &list);
 
 	drm_gem_object_unreference_unlocked(gobj);
 	return r;
-- 
2.5.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amdgpu: stop reserving a shared fence for VA updates

[Christian König]

From: Christian König <christian.koenig@amd.com>

We don't add any fences do the buffer, but just use it's address.

Additional to that we don't need a duplicates list here.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index ec5b184..9dd41e7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -549,7 +549,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
 	struct amdgpu_bo_list_entry vm_pd;
 	struct ttm_validate_buffer tv;
 	struct ww_acquire_ctx ticket;
-	struct list_head list, duplicates;
+	struct list_head list;
 	uint32_t invalid_flags, va_flags = 0;
 	int r = 0;
 
@@ -587,14 +587,13 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
 		return -ENOENT;
 	abo = gem_to_amdgpu_bo(gobj);
 	INIT_LIST_HEAD(&list);
-	INIT_LIST_HEAD(&duplicates);
 	tv.bo = &abo->tbo;
-	tv.shared = true;
+	tv.shared = false;
 	list_add(&tv.head, &list);
 
 	amdgpu_vm_get_pd_bo(&fpriv->vm, &list, &vm_pd);
 
-	r = ttm_eu_reserve_buffers(&ticket, &list, true, &duplicates);
+	r = ttm_eu_reserve_buffers(&ticket, &list, true, NULL);
 	if (r) {
 		drm_gem_object_unreference_unlocked(gobj);
 		return r;
-- 
2.5.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu: fix race in GEM VA map IOCTL

[Nicolai Hähnle]

On 30.01.2017 10:45, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> Somebody could try to free the bo_va between mapping and updating it.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>

Nice catch! Both patches:

Reviewed-by: Nicolai Hähnle <nicolai.haehnle@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 45 ++++++++++-----------------------
>  1 file changed, 14 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 7a265d9..ec5b184 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -489,67 +489,50 @@ static int amdgpu_gem_va_check(void *param, struct amdgpu_bo *bo)
>   *
>   * @adev: amdgpu_device pointer
>   * @bo_va: bo_va to update
> + * @list: validation list
> + * opration: map or unmap
>   *
>   * Update the bo_va directly after setting it's address. Errors are not
>   * vital here, so they are not reported back to userspace.
>   */
>  static void amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
>  				    struct amdgpu_bo_va *bo_va,
> +				    struct list_head *list,
>  				    uint32_t operation)
>  {
> -	struct ttm_validate_buffer tv, *entry;
> -	struct amdgpu_bo_list_entry vm_pd;
> -	struct ww_acquire_ctx ticket;
> -	struct list_head list, duplicates;
> -	int r;
> -
> -	INIT_LIST_HEAD(&list);
> -	INIT_LIST_HEAD(&duplicates);
> -
> -	tv.bo = &bo_va->bo->tbo;
> -	tv.shared = true;
> -	list_add(&tv.head, &list);
> -
> -	amdgpu_vm_get_pd_bo(bo_va->vm, &list, &vm_pd);
> -
> -	/* Provide duplicates to avoid -EALREADY */
> -	r = ttm_eu_reserve_buffers(&ticket, &list, true, &duplicates);
> -	if (r)
> -		goto error_print;
> +	struct ttm_validate_buffer *entry;
> +	int r = -ERESTARTSYS;
>
> -	list_for_each_entry(entry, &list, head) {
> +	list_for_each_entry(entry, list, head) {
>  		struct amdgpu_bo *bo =
>  			container_of(entry->bo, struct amdgpu_bo, tbo);
>
>  		/* if anything is swapped out don't swap it in here,
>  		   just abort and wait for the next CS */
>  		if (!amdgpu_bo_gpu_accessible(bo))
> -			goto error_unreserve;
> +			goto error;
>
>  		if (bo->shadow && !amdgpu_bo_gpu_accessible(bo->shadow))
> -			goto error_unreserve;
> +			goto error;
>  	}
>
>  	r = amdgpu_vm_validate_pt_bos(adev, bo_va->vm, amdgpu_gem_va_check,
>  				      NULL);
>  	if (r)
> -		goto error_unreserve;
> +		goto error;
>
>  	r = amdgpu_vm_update_page_directory(adev, bo_va->vm);
>  	if (r)
> -		goto error_unreserve;
> +		goto error;
>
>  	r = amdgpu_vm_clear_freed(adev, bo_va->vm);
>  	if (r)
> -		goto error_unreserve;
> +		goto error;
>
>  	if (operation == AMDGPU_VA_OP_MAP)
>  		r = amdgpu_vm_bo_update(adev, bo_va, false);
>
> -error_unreserve:
> -	ttm_eu_backoff_reservation(&ticket, &list);
> -
> -error_print:
> +error:
>  	if (r && r != -ERESTARTSYS)
>  		DRM_ERROR("Couldn't update BO_VA (%d)\n", r);
>  }
> @@ -642,10 +625,10 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>  	default:
>  		break;
>  	}
> -	ttm_eu_backoff_reservation(&ticket, &list);
>  	if (!r && !(args->flags & AMDGPU_VM_DELAY_UPDATE) &&
>  	    !amdgpu_vm_debug)
> -		amdgpu_gem_va_update_vm(adev, bo_va, args->operation);
> +		amdgpu_gem_va_update_vm(adev, bo_va, &list, args->operation);
> +	ttm_eu_backoff_reservation(&ticket, &list);
>
>  	drm_gem_object_unreference_unlocked(gobj);
>  	return r;
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx