Re: [PATCH] drm/tegra: Prevent BOs from being freed during job submission

2017-08-13 Thread Dmitry Osipenko 
On 11.08.2017 20:59, Thierry Reding wrote:
> From: Dmitry Osipenko 
> 
> Since DRM IOCTL's are lockless, there is a chance that BOs could be
> released while a job submission is in progress. To avoid that, keep the
> GEM reference until the job has been pinned, part of which will be to
> take another reference.
> 
> Signed-off-by: Dmitry Osipenko 
> Signed-off-by: Thierry Reding 
> ---
>  drivers/gpu/drm/tegra/drm.c | 42 +-
>  1 file changed, 33 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index f01db33fa20f..e5d19e1c9bc8 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -320,8 +320,6 @@ host1x_bo_lookup(struct drm_file *file, u32 handle)
>   if (!gem)
>   return NULL;
>  
> - drm_gem_object_unreference_unlocked(gem);
> -
>   bo = to_tegra_bo(gem);
>   return >base;
>  }
> @@ -410,8 +408,10 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>   (void __user *)(uintptr_t)args->waitchks;
>   struct drm_tegra_syncpt syncpt;
>   struct host1x *host1x = dev_get_drvdata(drm->dev->parent);
> + struct drm_gem_object **refs;

Should be "**refs = NULL" in conjunction with a missed kfree() below.

>   struct host1x_syncpt *sp;
>   struct host1x_job *job;
> + unsigned int num_refs;
>   int err;
>  
>   /* We don't yet support other than one syncpt_incr struct per submit */
> @@ -433,6 +433,26 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>   job->class = context->client->base.class;
>   job->serialize = true;
>  
> + /*
> +  * Track referenced BOs so that they can be unreferenced after the
> +  * submission is complete.
> +  */
> + num_refs = num_cmdbufs + num_relocs * 2 + num_waitchks;
> +
> + if (sizeof(*refs) * num_refs > ULONG_MAX) {
> + err = -EINVAL;
> + goto fail;
> + }
> +
> + refs = kmalloc_array(num_refs, sizeof(*refs), GFP_KERNEL);
> + if (!refs) {
> + err = -ENOMEM;
> + goto fail;
> + }
> +
> + /* reuse as an iterator later */
> + num_refs = 0;
> +
>   while (num_cmdbufs) {
>   struct drm_tegra_cmdbuf cmdbuf;
>   struct host1x_bo *bo;
> @@ -461,6 +481,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>  
>   offset = (u64)cmdbuf.offset + (u64)cmdbuf.words * sizeof(u32);
>   obj = host1x_to_tegra_bo(bo);
> + refs[num_refs++] = >gem;
>  
>   /*
>* Gather buffer base address must be 4-bytes aligned,
> @@ -490,6 +511,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>  
>   reloc = >relocarray[num_relocs];
>   obj = host1x_to_tegra_bo(reloc->cmdbuf.bo);
> + refs[num_refs++] = >gem;
>  
>   /*
>* The unaligned cmdbuf offset will cause an unaligned write
> @@ -503,6 +525,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>   }
>  
>   obj = host1x_to_tegra_bo(reloc->target.bo);
> + refs[num_refs++] = >gem;
>  
>   if (reloc->target.offset >= obj->gem.size) {
>   err = -EINVAL;
> @@ -522,6 +545,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>   goto fail;
>  
>   obj = host1x_to_tegra_bo(wait->bo);
> + refs[num_refs++] = >gem;
>  
>   /*
>* The unaligned offset will cause an unaligned write during
> @@ -561,17 +585,17 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>   goto fail;
>  
>   err = host1x_job_submit(job);
> - if (err)
> - goto fail_submit;
> + if (err) {
> + host1x_job_unpin(job);
> + goto fail;
> + }
>  
>   args->fence = job->syncpt_end;
>  
> - host1x_job_put(job);
> - return 0;
> -
> -fail_submit:
> - host1x_job_unpin(job);
>  fail:
> + while (num_refs--)
> + drm_gem_object_put_unlocked(refs[num_refs]);
> +

kfree(refs) is missed here.

>   host1x_job_put(job);
>   return err;
>  }
> 


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

Re: [PATCH] drm/tegra: Prevent BOs from being freed during job submission

2017-08-13 Thread Dmitry Osipenko 
On 11.08.2017 21:16, Dmitry Osipenko wrote:
> On 11.08.2017 20:59, Thierry Reding wrote:
>> From: Dmitry Osipenko 
>>
>> Since DRM IOCTL's are lockless, there is a chance that BOs could be
>> released while a job submission is in progress. To avoid that, keep the
>> GEM reference until the job has been pinned, part of which will be to
>> take another reference.
>>
>> Signed-off-by: Dmitry Osipenko 
>> Signed-off-by: Thierry Reding 
>> ---
>>  drivers/gpu/drm/tegra/drm.c | 42 +-
>>  1 file changed, 33 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
>> index f01db33fa20f..e5d19e1c9bc8 100644
>> --- a/drivers/gpu/drm/tegra/drm.c
>> +++ b/drivers/gpu/drm/tegra/drm.c
>> @@ -320,8 +320,6 @@ host1x_bo_lookup(struct drm_file *file, u32 handle)
>>  if (!gem)
>>  return NULL;
>>  
>> -drm_gem_object_unreference_unlocked(gem);
>> -
>>  bo = to_tegra_bo(gem);
>>  return >base;
>>  }
>> @@ -410,8 +408,10 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>>  (void __user *)(uintptr_t)args->waitchks;
>>  struct drm_tegra_syncpt syncpt;
>>  struct host1x *host1x = dev_get_drvdata(drm->dev->parent);
>> +struct drm_gem_object **refs;
>>  struct host1x_syncpt *sp;
>>  struct host1x_job *job;
>> +unsigned int num_refs;
>>  int err;
>>  
>>  /* We don't yet support other than one syncpt_incr struct per submit */
>> @@ -433,6 +433,26 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>>  job->class = context->client->base.class;
>>  job->serialize = true;
>>  
>> +/*
>> + * Track referenced BOs so that they can be unreferenced after the
>> + * submission is complete.
>> + */
>> +num_refs = num_cmdbufs + num_relocs * 2 + num_waitchks;
>> +
>> +if (sizeof(*refs) * num_refs > ULONG_MAX) {
> 
> Thierry, since you've omitted (u64) here (comparing to the original patch), I
> think it should be better to write as:
> 
>   if (num_refs > ULONG_MAX / sizeof(*refs)) {
> 
> To avoid integer overflow in the check.
> 

Moreover, this check isn't needed at all because it duplicates same checking
done by kmalloc_array().

>> +err = -EINVAL;
>> +goto fail;
>> +}
>> +
>> +refs = kmalloc_array(num_refs, sizeof(*refs), GFP_KERNEL);
>> +if (!refs) {
>> +err = -ENOMEM;
>> +goto fail;
>> +}
>> +
>> +/* reuse as an iterator later */
>> +num_refs = 0;
>> +
>>  while (num_cmdbufs) {
>>  struct drm_tegra_cmdbuf cmdbuf;
>>  struct host1x_bo *bo;
>> @@ -461,6 +481,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>>  
>>  offset = (u64)cmdbuf.offset + (u64)cmdbuf.words * sizeof(u32);
>>  obj = host1x_to_tegra_bo(bo);
>> +refs[num_refs++] = >gem;
>>  
>>  /*
>>   * Gather buffer base address must be 4-bytes aligned,
>> @@ -490,6 +511,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>>  
>>  reloc = >relocarray[num_relocs];
>>  obj = host1x_to_tegra_bo(reloc->cmdbuf.bo);
>> +refs[num_refs++] = >gem;
>>  
>>  /*
>>   * The unaligned cmdbuf offset will cause an unaligned write
>> @@ -503,6 +525,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>>  }
>>  
>>  obj = host1x_to_tegra_bo(reloc->target.bo);
>> +refs[num_refs++] = >gem;
>>  
>>  if (reloc->target.offset >= obj->gem.size) {
>>  err = -EINVAL;
>> @@ -522,6 +545,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>>  goto fail;
>>  
>>  obj = host1x_to_tegra_bo(wait->bo);
>> +refs[num_refs++] = >gem;
>>  
>>  /*
>>   * The unaligned offset will cause an unaligned write during
>> @@ -561,17 +585,17 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>>  goto fail;
>>  
>>  err = host1x_job_submit(job);
>> -if (err)
>> -goto fail_submit;
>> +if (err) {
>> +host1x_job_unpin(job);
>> +goto fail;
>> +}
>>  
>>  args->fence = job->syncpt_end;
>>  
>> -host1x_job_put(job);
>> -return 0;
>> -
>> -fail_submit:
>> -host1x_job_unpin(job);
>>  fail:
>> +while (num_refs--)
>> +drm_gem_object_put_unlocked(refs[num_refs]);
>> +
>>  host1x_job_put(job);
>>  return err;
>>  }
>>
> 
> 


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

Re: [PATCH] drm/tegra: Prevent BOs from being freed during job submission

2017-08-12 Thread Dmitry Osipenko 
On 11.08.2017 20:59, Thierry Reding wrote:
> From: Dmitry Osipenko 
> 
> Since DRM IOCTL's are lockless, there is a chance that BOs could be
> released while a job submission is in progress. To avoid that, keep the
> GEM reference until the job has been pinned, part of which will be to
> take another reference.
> 
> Signed-off-by: Dmitry Osipenko 
> Signed-off-by: Thierry Reding 
> ---
>  drivers/gpu/drm/tegra/drm.c | 42 +-
>  1 file changed, 33 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index f01db33fa20f..e5d19e1c9bc8 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -320,8 +320,6 @@ host1x_bo_lookup(struct drm_file *file, u32 handle)
>   if (!gem)
>   return NULL;
>  
> - drm_gem_object_unreference_unlocked(gem);
> -
>   bo = to_tegra_bo(gem);
>   return >base;
>  }
> @@ -410,8 +408,10 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>   (void __user *)(uintptr_t)args->waitchks;
>   struct drm_tegra_syncpt syncpt;
>   struct host1x *host1x = dev_get_drvdata(drm->dev->parent);
> + struct drm_gem_object **refs;
>   struct host1x_syncpt *sp;
>   struct host1x_job *job;
> + unsigned int num_refs;
>   int err;
>  
>   /* We don't yet support other than one syncpt_incr struct per submit */
> @@ -433,6 +433,26 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>   job->class = context->client->base.class;
>   job->serialize = true;
>  
> + /*
> +  * Track referenced BOs so that they can be unreferenced after the
> +  * submission is complete.
> +  */
> + num_refs = num_cmdbufs + num_relocs * 2 + num_waitchks;
> +
> + if (sizeof(*refs) * num_refs > ULONG_MAX) {

Thierry, since you've omitted (u64) here (comparing to the original patch), I
think it should be better to write as:

if (num_refs > ULONG_MAX / sizeof(*refs)) {

To avoid integer overflow in the check.

> + err = -EINVAL;
> + goto fail;
> + }
> +
> + refs = kmalloc_array(num_refs, sizeof(*refs), GFP_KERNEL);
> + if (!refs) {
> + err = -ENOMEM;
> + goto fail;
> + }
> +
> + /* reuse as an iterator later */
> + num_refs = 0;
> +
>   while (num_cmdbufs) {
>   struct drm_tegra_cmdbuf cmdbuf;
>   struct host1x_bo *bo;
> @@ -461,6 +481,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>  
>   offset = (u64)cmdbuf.offset + (u64)cmdbuf.words * sizeof(u32);
>   obj = host1x_to_tegra_bo(bo);
> + refs[num_refs++] = >gem;
>  
>   /*
>* Gather buffer base address must be 4-bytes aligned,
> @@ -490,6 +511,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>  
>   reloc = >relocarray[num_relocs];
>   obj = host1x_to_tegra_bo(reloc->cmdbuf.bo);
> + refs[num_refs++] = >gem;
>  
>   /*
>* The unaligned cmdbuf offset will cause an unaligned write
> @@ -503,6 +525,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>   }
>  
>   obj = host1x_to_tegra_bo(reloc->target.bo);
> + refs[num_refs++] = >gem;
>  
>   if (reloc->target.offset >= obj->gem.size) {
>   err = -EINVAL;
> @@ -522,6 +545,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>   goto fail;
>  
>   obj = host1x_to_tegra_bo(wait->bo);
> + refs[num_refs++] = >gem;
>  
>   /*
>* The unaligned offset will cause an unaligned write during
> @@ -561,17 +585,17 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>   goto fail;
>  
>   err = host1x_job_submit(job);
> - if (err)
> - goto fail_submit;
> + if (err) {
> + host1x_job_unpin(job);
> + goto fail;
> + }
>  
>   args->fence = job->syncpt_end;
>  
> - host1x_job_put(job);
> - return 0;
> -
> -fail_submit:
> - host1x_job_unpin(job);
>  fail:
> + while (num_refs--)
> + drm_gem_object_put_unlocked(refs[num_refs]);
> +
>   host1x_job_put(job);
>   return err;
>  }
> 


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

[PATCH] drm/tegra: Prevent BOs from being freed during job submission

2017-08-11 Thread Thierry Reding 
From: Dmitry Osipenko 

Since DRM IOCTL's are lockless, there is a chance that BOs could be
released while a job submission is in progress. To avoid that, keep the
GEM reference until the job has been pinned, part of which will be to
take another reference.

Signed-off-by: Dmitry Osipenko 
Signed-off-by: Thierry Reding 
---
 drivers/gpu/drm/tegra/drm.c | 42 +-
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index f01db33fa20f..e5d19e1c9bc8 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -320,8 +320,6 @@ host1x_bo_lookup(struct drm_file *file, u32 handle)
if (!gem)
return NULL;
 
-   drm_gem_object_unreference_unlocked(gem);
-
bo = to_tegra_bo(gem);
return >base;
 }
@@ -410,8 +408,10 @@ int tegra_drm_submit(struct tegra_drm_context *context,
(void __user *)(uintptr_t)args->waitchks;
struct drm_tegra_syncpt syncpt;
struct host1x *host1x = dev_get_drvdata(drm->dev->parent);
+   struct drm_gem_object **refs;
struct host1x_syncpt *sp;
struct host1x_job *job;
+   unsigned int num_refs;
int err;
 
/* We don't yet support other than one syncpt_incr struct per submit */
@@ -433,6 +433,26 @@ int tegra_drm_submit(struct tegra_drm_context *context,
job->class = context->client->base.class;
job->serialize = true;
 
+   /*
+* Track referenced BOs so that they can be unreferenced after the
+* submission is complete.
+*/
+   num_refs = num_cmdbufs + num_relocs * 2 + num_waitchks;
+
+   if (sizeof(*refs) * num_refs > ULONG_MAX) {
+   err = -EINVAL;
+   goto fail;
+   }
+
+   refs = kmalloc_array(num_refs, sizeof(*refs), GFP_KERNEL);
+   if (!refs) {
+   err = -ENOMEM;
+   goto fail;
+   }
+
+   /* reuse as an iterator later */
+   num_refs = 0;
+
while (num_cmdbufs) {
struct drm_tegra_cmdbuf cmdbuf;
struct host1x_bo *bo;
@@ -461,6 +481,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
 
offset = (u64)cmdbuf.offset + (u64)cmdbuf.words * sizeof(u32);
obj = host1x_to_tegra_bo(bo);
+   refs[num_refs++] = >gem;
 
/*
 * Gather buffer base address must be 4-bytes aligned,
@@ -490,6 +511,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
 
reloc = >relocarray[num_relocs];
obj = host1x_to_tegra_bo(reloc->cmdbuf.bo);
+   refs[num_refs++] = >gem;
 
/*
 * The unaligned cmdbuf offset will cause an unaligned write
@@ -503,6 +525,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
}
 
obj = host1x_to_tegra_bo(reloc->target.bo);
+   refs[num_refs++] = >gem;
 
if (reloc->target.offset >= obj->gem.size) {
err = -EINVAL;
@@ -522,6 +545,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
goto fail;
 
obj = host1x_to_tegra_bo(wait->bo);
+   refs[num_refs++] = >gem;
 
/*
 * The unaligned offset will cause an unaligned write during
@@ -561,17 +585,17 @@ int tegra_drm_submit(struct tegra_drm_context *context,
goto fail;
 
err = host1x_job_submit(job);
-   if (err)
-   goto fail_submit;
+   if (err) {
+   host1x_job_unpin(job);
+   goto fail;
+   }
 
args->fence = job->syncpt_end;
 
-   host1x_job_put(job);
-   return 0;
-
-fail_submit:
-   host1x_job_unpin(job);
 fail:
+   while (num_refs--)
+   drm_gem_object_put_unlocked(refs[num_refs]);
+
host1x_job_put(job);
return err;
 }
-- 
2.13.3

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