All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/amdgpu: Protect the validate list with a mutex
       [not found] <a0ae6ae0-35ec-7da9-0671-42bdf126460b@amd.com>
@ 2022-06-23  4:25 ` Luben Tuikov
  2022-06-23  4:39   ` [PATCH v2] " Luben Tuikov
  0 siblings, 1 reply; 3+ messages in thread
From: Luben Tuikov @ 2022-06-23  4:25 UTC (permalink / raw)
  To: amd-gfx
  Cc: Alex Deucher, Andrey Grodzovsky, Luben Tuikov,
	Christian König, Vitaly Prosyak

Protect the parser's validate list with a mutex in order to avoid buffer
object corruption as recorded in the link below.

Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <Alexander.Deucher@amd.com>
Cc: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
Cc: Vitaly Prosyak <Vitaly.Prosyak@amd.com>
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2048
Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 28 +++++++++++++++++++++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.h |  4 ++++
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 36ac1f1d11e6b4..e593e30a8545f9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -498,6 +498,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 	struct amdgpu_bo *oa;
 	int r;
 
+	mutex_init(&p->mutex_validated);
 	INIT_LIST_HEAD(&p->validated);
 
 	/* p->bo_list could already be assigned if AMDGPU_CHUNK_ID_BO_HANDLES is present */
@@ -521,13 +522,15 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 	amdgpu_bo_list_for_each_entry(e, p->bo_list)
 		e->tv.num_shared = 2;
 
-	amdgpu_bo_list_get_list(p->bo_list, &p->validated);
-
 	INIT_LIST_HEAD(&duplicates);
+
+	mutex_lock(&p->mutex_validated);
+	amdgpu_bo_list_get_list(p->bo_list, &p->validated);
 	amdgpu_vm_get_pd_bo(&fpriv->vm, &p->validated, &p->vm_pd);
 
 	if (p->uf_entry.tv.bo && !ttm_to_amdgpu_bo(p->uf_entry.tv.bo)->parent)
 		list_add(&p->uf_entry.tv.head, &p->validated);
+	mutex_unlock(&p->mutex_validated);
 
 	/* Get userptr backing pages. If pages are updated after registered
 	 * in amdgpu_gem_userptr_ioctl(), amdgpu_cs_list_validate() will do
@@ -563,8 +566,11 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 		e->user_invalidated = userpage_invalidated;
 	}
 
+	mutex_lock(&p->mutex_validated);
 	r = ttm_eu_reserve_buffers(&p->ticket, &p->validated, true,
 				   &duplicates);
+	mutex_unlock(&p->mutex_validated);
+
 	if (unlikely(r != 0)) {
 		if (r != -ERESTARTSYS)
 			DRM_ERROR("ttm_eu_reserve_buffers failed.\n");
@@ -607,11 +613,15 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 		goto error_validate;
 	}
 
+	mutex_lock(&p->mutex_validated);
 	r = amdgpu_cs_list_validate(p, &duplicates);
-	if (r)
+	if (r) {
+		mutex_unlock(&p->mutex_validated);
 		goto error_validate;
+	}
 
 	r = amdgpu_cs_list_validate(p, &p->validated);
+	mutex_unlock(&p->mutex_validated);
 	if (r)
 		goto error_validate;
 
@@ -648,7 +658,9 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 			dma_fence_chain_free(e->chain);
 			e->chain = NULL;
 		}
+		mutex_lock(&p->mutex_validated);
 		ttm_eu_backoff_reservation(&p->ticket, &p->validated);
+		mutex_unlock(&p->mutex_validated);
 	}
 
 out_free_user_pages:
@@ -672,6 +684,7 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p)
 	struct amdgpu_bo_list_entry *e;
 	int r;
 
+	mutex_lock(&p->mutex_validated);
 	list_for_each_entry(e, &p->validated, tv.head) {
 		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
 		struct dma_resv *resv = bo->tbo.base.resv;
@@ -682,9 +695,10 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p)
 		r = amdgpu_sync_resv(p->adev, &p->job->sync, resv, sync_mode,
 				     &fpriv->vm);
 		if (r)
-			return r;
+			break;
 	}
-	return 0;
+	mutex_unlock(&p->mutex_validated);
+	return r ?: 0;
 }
 
 /**
@@ -709,8 +723,10 @@ static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser, int error,
 			e->chain = NULL;
 		}
 
+		mutex_lock(&parser->mutex_validated);
 		ttm_eu_backoff_reservation(&parser->ticket,
 					   &parser->validated);
+		mutex_unlock(&parser->mutex_validated);
 	}
 
 	for (i = 0; i < parser->num_post_deps; i++) {
@@ -1307,7 +1323,9 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
 		e->chain = NULL;
 	}
 
+	mutex_lock(&p->mutex_validated);
 	ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence);
+	mutex_unlock(&p->mutex_validated);
 	mutex_unlock(&p->adev->notifier_lock);
 
 	return 0;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.h
index 30ecc4917f811d..284d1c03d65d0b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.h
@@ -59,6 +59,10 @@ struct amdgpu_cs_parser {
 	struct amdgpu_bo_list		*bo_list;
 	struct amdgpu_mn		*mn;
 	struct amdgpu_bo_list_entry	vm_pd;
+
+	/* Protect access to list "valided" below.
+	 */
+	struct mutex                    mutex_validated;
 	struct list_head		validated;
 	struct dma_fence		*fence;
 	uint64_t			bytes_moved_threshold;

base-commit: ab7e60938be74e21c723223e7eb96cac7b441e5e
-- 
2.36.1.74.g277cf0bc36


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* [PATCH v2] drm/amdgpu: Protect the validate list with a mutex
  2022-06-23  4:25 ` [PATCH] drm/amdgpu: Protect the validate list with a mutex Luben Tuikov
@ 2022-06-23  4:39   ` Luben Tuikov
  2022-06-23  6:31     ` Christian König
  0 siblings, 1 reply; 3+ messages in thread
From: Luben Tuikov @ 2022-06-23  4:39 UTC (permalink / raw)
  To: amd-gfx
  Cc: Alex Deucher, Andrey Grodzovsky, Luben Tuikov,
	Christian König, Vitaly Prosyak

Protect the parser's validate list with a mutex in order to avoid buffer
object corruption as recorded in the link below.

Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <Alexander.Deucher@amd.com>
Cc: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
Cc: Vitaly Prosyak <Vitaly.Prosyak@amd.com>
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2048
Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 30 ++++++++++++++++++++------
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.h |  4 ++++
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 36ac1f1d11e6b4..0be0bf17c05420 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -498,6 +498,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 	struct amdgpu_bo *oa;
 	int r;
 
+	mutex_init(&p->mutex_validated);
 	INIT_LIST_HEAD(&p->validated);
 
 	/* p->bo_list could already be assigned if AMDGPU_CHUNK_ID_BO_HANDLES is present */
@@ -521,13 +522,15 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 	amdgpu_bo_list_for_each_entry(e, p->bo_list)
 		e->tv.num_shared = 2;
 
-	amdgpu_bo_list_get_list(p->bo_list, &p->validated);
-
 	INIT_LIST_HEAD(&duplicates);
+
+	mutex_lock(&p->mutex_validated);
+	amdgpu_bo_list_get_list(p->bo_list, &p->validated);
 	amdgpu_vm_get_pd_bo(&fpriv->vm, &p->validated, &p->vm_pd);
 
 	if (p->uf_entry.tv.bo && !ttm_to_amdgpu_bo(p->uf_entry.tv.bo)->parent)
 		list_add(&p->uf_entry.tv.head, &p->validated);
+	mutex_unlock(&p->mutex_validated);
 
 	/* Get userptr backing pages. If pages are updated after registered
 	 * in amdgpu_gem_userptr_ioctl(), amdgpu_cs_list_validate() will do
@@ -563,8 +566,11 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 		e->user_invalidated = userpage_invalidated;
 	}
 
+	mutex_lock(&p->mutex_validated);
 	r = ttm_eu_reserve_buffers(&p->ticket, &p->validated, true,
 				   &duplicates);
+	mutex_unlock(&p->mutex_validated);
+
 	if (unlikely(r != 0)) {
 		if (r != -ERESTARTSYS)
 			DRM_ERROR("ttm_eu_reserve_buffers failed.\n");
@@ -607,11 +613,15 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 		goto error_validate;
 	}
 
+	mutex_lock(&p->mutex_validated);
 	r = amdgpu_cs_list_validate(p, &duplicates);
-	if (r)
+	if (r) {
+		mutex_unlock(&p->mutex_validated);
 		goto error_validate;
+	}
 
 	r = amdgpu_cs_list_validate(p, &p->validated);
+	mutex_unlock(&p->mutex_validated);
 	if (r)
 		goto error_validate;
 
@@ -648,7 +658,9 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 			dma_fence_chain_free(e->chain);
 			e->chain = NULL;
 		}
+		mutex_lock(&p->mutex_validated);
 		ttm_eu_backoff_reservation(&p->ticket, &p->validated);
+		mutex_unlock(&p->mutex_validated);
 	}
 
 out_free_user_pages:
@@ -670,8 +682,9 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p)
 {
 	struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
 	struct amdgpu_bo_list_entry *e;
-	int r;
+	int r = 0;
 
+	mutex_lock(&p->mutex_validated);
 	list_for_each_entry(e, &p->validated, tv.head) {
 		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
 		struct dma_resv *resv = bo->tbo.base.resv;
@@ -682,9 +695,10 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p)
 		r = amdgpu_sync_resv(p->adev, &p->job->sync, resv, sync_mode,
 				     &fpriv->vm);
 		if (r)
-			return r;
+			break;
 	}
-	return 0;
+	mutex_unlock(&p->mutex_validated);
+	return r;
 }
 
 /**
@@ -709,8 +723,10 @@ static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser, int error,
 			e->chain = NULL;
 		}
 
+		mutex_lock(&parser->mutex_validated);
 		ttm_eu_backoff_reservation(&parser->ticket,
 					   &parser->validated);
+		mutex_unlock(&parser->mutex_validated);
 	}
 
 	for (i = 0; i < parser->num_post_deps; i++) {
@@ -1307,7 +1323,9 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
 		e->chain = NULL;
 	}
 
+	mutex_lock(&p->mutex_validated);
 	ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence);
+	mutex_unlock(&p->mutex_validated);
 	mutex_unlock(&p->adev->notifier_lock);
 
 	return 0;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.h
index 30ecc4917f811d..284d1c03d65d0b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.h
@@ -59,6 +59,10 @@ struct amdgpu_cs_parser {
 	struct amdgpu_bo_list		*bo_list;
 	struct amdgpu_mn		*mn;
 	struct amdgpu_bo_list_entry	vm_pd;
+
+	/* Protect access to list "valided" below.
+	 */
+	struct mutex                    mutex_validated;
 	struct list_head		validated;
 	struct dma_fence		*fence;
 	uint64_t			bytes_moved_threshold;

base-commit: ab7e60938be74e21c723223e7eb96cac7b441e5e
-- 
2.36.1.74.g277cf0bc36


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH v2] drm/amdgpu: Protect the validate list with a mutex
  2022-06-23  4:39   ` [PATCH v2] " Luben Tuikov
@ 2022-06-23  6:31     ` Christian König
  0 siblings, 0 replies; 3+ messages in thread
From: Christian König @ 2022-06-23  6:31 UTC (permalink / raw)
  To: Luben Tuikov, amd-gfx; +Cc: Alex Deucher, Andrey Grodzovsky, Vitaly Prosyak

The mutex must be added to the bo_list structure, not the parser structure.

The parser is only a temporary structure we allocate for the current thread.

Regards,
Christian.

Am 23.06.22 um 06:39 schrieb Luben Tuikov:
> Protect the parser's validate list with a mutex in order to avoid buffer
> object corruption as recorded in the link below.
>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <Alexander.Deucher@amd.com>
> Cc: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
> Cc: Vitaly Prosyak <Vitaly.Prosyak@amd.com>
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2048
> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 30 ++++++++++++++++++++------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.h |  4 ++++
>   2 files changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 36ac1f1d11e6b4..0be0bf17c05420 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -498,6 +498,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>   	struct amdgpu_bo *oa;
>   	int r;
>   
> +	mutex_init(&p->mutex_validated);
>   	INIT_LIST_HEAD(&p->validated);
>   
>   	/* p->bo_list could already be assigned if AMDGPU_CHUNK_ID_BO_HANDLES is present */
> @@ -521,13 +522,15 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>   	amdgpu_bo_list_for_each_entry(e, p->bo_list)
>   		e->tv.num_shared = 2;
>   
> -	amdgpu_bo_list_get_list(p->bo_list, &p->validated);
> -
>   	INIT_LIST_HEAD(&duplicates);
> +
> +	mutex_lock(&p->mutex_validated);
> +	amdgpu_bo_list_get_list(p->bo_list, &p->validated);
>   	amdgpu_vm_get_pd_bo(&fpriv->vm, &p->validated, &p->vm_pd);
>   
>   	if (p->uf_entry.tv.bo && !ttm_to_amdgpu_bo(p->uf_entry.tv.bo)->parent)
>   		list_add(&p->uf_entry.tv.head, &p->validated);
> +	mutex_unlock(&p->mutex_validated);
>   
>   	/* Get userptr backing pages. If pages are updated after registered
>   	 * in amdgpu_gem_userptr_ioctl(), amdgpu_cs_list_validate() will do
> @@ -563,8 +566,11 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>   		e->user_invalidated = userpage_invalidated;
>   	}
>   
> +	mutex_lock(&p->mutex_validated);
>   	r = ttm_eu_reserve_buffers(&p->ticket, &p->validated, true,
>   				   &duplicates);
> +	mutex_unlock(&p->mutex_validated);
> +
>   	if (unlikely(r != 0)) {
>   		if (r != -ERESTARTSYS)
>   			DRM_ERROR("ttm_eu_reserve_buffers failed.\n");
> @@ -607,11 +613,15 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>   		goto error_validate;
>   	}
>   
> +	mutex_lock(&p->mutex_validated);
>   	r = amdgpu_cs_list_validate(p, &duplicates);
> -	if (r)
> +	if (r) {
> +		mutex_unlock(&p->mutex_validated);
>   		goto error_validate;
> +	}
>   
>   	r = amdgpu_cs_list_validate(p, &p->validated);
> +	mutex_unlock(&p->mutex_validated);
>   	if (r)
>   		goto error_validate;
>   
> @@ -648,7 +658,9 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>   			dma_fence_chain_free(e->chain);
>   			e->chain = NULL;
>   		}
> +		mutex_lock(&p->mutex_validated);
>   		ttm_eu_backoff_reservation(&p->ticket, &p->validated);
> +		mutex_unlock(&p->mutex_validated);
>   	}
>   
>   out_free_user_pages:
> @@ -670,8 +682,9 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p)
>   {
>   	struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
>   	struct amdgpu_bo_list_entry *e;
> -	int r;
> +	int r = 0;
>   
> +	mutex_lock(&p->mutex_validated);
>   	list_for_each_entry(e, &p->validated, tv.head) {
>   		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
>   		struct dma_resv *resv = bo->tbo.base.resv;
> @@ -682,9 +695,10 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p)
>   		r = amdgpu_sync_resv(p->adev, &p->job->sync, resv, sync_mode,
>   				     &fpriv->vm);
>   		if (r)
> -			return r;
> +			break;
>   	}
> -	return 0;
> +	mutex_unlock(&p->mutex_validated);
> +	return r;
>   }
>   
>   /**
> @@ -709,8 +723,10 @@ static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser, int error,
>   			e->chain = NULL;
>   		}
>   
> +		mutex_lock(&parser->mutex_validated);
>   		ttm_eu_backoff_reservation(&parser->ticket,
>   					   &parser->validated);
> +		mutex_unlock(&parser->mutex_validated);
>   	}
>   
>   	for (i = 0; i < parser->num_post_deps; i++) {
> @@ -1307,7 +1323,9 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>   		e->chain = NULL;
>   	}
>   
> +	mutex_lock(&p->mutex_validated);
>   	ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence);
> +	mutex_unlock(&p->mutex_validated);
>   	mutex_unlock(&p->adev->notifier_lock);
>   
>   	return 0;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.h
> index 30ecc4917f811d..284d1c03d65d0b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.h
> @@ -59,6 +59,10 @@ struct amdgpu_cs_parser {
>   	struct amdgpu_bo_list		*bo_list;
>   	struct amdgpu_mn		*mn;
>   	struct amdgpu_bo_list_entry	vm_pd;
> +
> +	/* Protect access to list "valided" below.
> +	 */
> +	struct mutex                    mutex_validated;
>   	struct list_head		validated;
>   	struct dma_fence		*fence;
>   	uint64_t			bytes_moved_threshold;
>
> base-commit: ab7e60938be74e21c723223e7eb96cac7b441e5e


^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-06-23  6:31 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <a0ae6ae0-35ec-7da9-0671-42bdf126460b@amd.com>
2022-06-23  4:25 ` [PATCH] drm/amdgpu: Protect the validate list with a mutex Luben Tuikov
2022-06-23  4:39   ` [PATCH v2] " Luben Tuikov
2022-06-23  6:31     ` Christian König

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.