[Intel-gfx] [PATCH 1/2] drm/i915: Fix a potential UAF at device unload

[Intel-gfx] [PATCH 1/2] drm/i915: Fix a potential UAF at device unload

[Nirmoy Das]

i915_gem_drain_freed_objects() might not be enough to
free all the objects and RCU delayed work might get
scheduled after the i915 device struct gets freed.

Call i915_gem_drain_workqueue() to catch all RCU delayed work.

Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0f49ec9d494a..e8a053eaaa89 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1254,7 +1254,7 @@ void i915_gem_init_early(struct drm_i915_private *dev_priv)
 
 void i915_gem_cleanup_early(struct drm_i915_private *dev_priv)
 {
-	i915_gem_drain_freed_objects(dev_priv);
+	i915_gem_drain_workqueue(dev_priv);
 	GEM_BUG_ON(!llist_empty(&dev_priv->mm.free_list));
 	GEM_BUG_ON(atomic_read(&dev_priv->mm.free_count));
 	drm_WARN_ON(&dev_priv->drm, dev_priv->mm.shrink_count);
-- 
2.37.3

[Intel-gfx] [PATCH 2/2] drm/i915: remove excessive i915_gem_drain_freed_objects

[Nirmoy Das]

i915_gem_drain_workqueue() call i915_gem_drain_freed_objects()
so no need to call that again.

Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c                  | 2 --
 drivers/gpu/drm/i915/selftests/mock_gem_device.c | 1 -
 2 files changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e8a053eaaa89..e16718d79533 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1217,8 +1217,6 @@ void i915_gem_driver_remove(struct drm_i915_private *dev_priv)
 
 	/* Flush any outstanding unpin_work. */
 	i915_gem_drain_workqueue(dev_priv);
-
-	i915_gem_drain_freed_objects(dev_priv);
 }
 
 void i915_gem_driver_release(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
index f5904e659ef2..5d02346c43a2 100644
--- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
+++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
@@ -67,7 +67,6 @@ static void mock_device_release(struct drm_device *dev)
 	intel_gt_driver_remove(to_gt(i915));
 
 	i915_gem_drain_workqueue(i915);
-	i915_gem_drain_freed_objects(i915);
 
 	mock_fini_ggtt(to_gt(i915)->ggtt);
 	destroy_workqueue(i915->wq);
-- 
2.37.3

[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915: Fix a potential UAF at device unload

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 7006 bytes --]

== Series Details ==

Series: series starting with [1/2] drm/i915: Fix a potential UAF at device unload
URL   : https://patchwork.freedesktop.org/series/108314/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_12100 -> Patchwork_108314v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_108314v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_108314v1, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108314v1/index.html

Participating hosts (42 -> 41)
------------------------------

  Missing    (1): fi-bdw-samus 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_108314v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@mman:
    - fi-rkl-guc:         [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12100/fi-rkl-guc/igt@i915_selftest@live@mman.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108314v1/fi-rkl-guc/igt@i915_selftest@live@mman.html

  
Known issues
------------

  Here are the changes found in Patchwork_108314v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@hangcheck:
    - fi-hsw-g3258:       [PASS][3] -> [INCOMPLETE][4] ([i915#3303] / [i915#4785])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12100/fi-hsw-g3258/igt@i915_selftest@live@hangcheck.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108314v1/fi-hsw-g3258/igt@i915_selftest@live@hangcheck.html
    - bat-dg1-5:          NOTRUN -> [DMESG-FAIL][5] ([i915#4494] / [i915#4957])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108314v1/bat-dg1-5/igt@i915_selftest@live@hangcheck.html
    - fi-snb-2600:        [PASS][6] -> [INCOMPLETE][7] ([i915#3921])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12100/fi-snb-2600/igt@i915_selftest@live@hangcheck.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108314v1/fi-snb-2600/igt@i915_selftest@live@hangcheck.html

  * igt@i915_suspend@basic-s2idle-without-i915:
    - bat-dg1-5:          NOTRUN -> [INCOMPLETE][8] ([i915#6011])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108314v1/bat-dg1-5/igt@i915_suspend@basic-s2idle-without-i915.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-hsw-4770:        NOTRUN -> [SKIP][9] ([fdo#109271] / [fdo#111827])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108314v1/fi-hsw-4770/igt@kms_chamelium@common-hpd-after-suspend.html
    - fi-blb-e6850:       NOTRUN -> [SKIP][10] ([fdo#109271])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108314v1/fi-blb-e6850/igt@kms_chamelium@common-hpd-after-suspend.html

  * igt@runner@aborted:
    - fi-hsw-g3258:       NOTRUN -> [FAIL][11] ([fdo#109271] / [i915#4312] / [i915#6246])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108314v1/fi-hsw-g3258/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gem:
    - fi-blb-e6850:       [DMESG-FAIL][12] ([i915#4528]) -> [PASS][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12100/fi-blb-e6850/igt@i915_selftest@live@gem.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108314v1/fi-blb-e6850/igt@i915_selftest@live@gem.html

  * igt@i915_selftest@live@gt_engines:
    - bat-dg1-5:          [INCOMPLETE][14] ([i915#4418]) -> [PASS][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12100/bat-dg1-5/igt@i915_selftest@live@gt_engines.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108314v1/bat-dg1-5/igt@i915_selftest@live@gt_engines.html

  * igt@i915_selftest@live@hangcheck:
    - fi-hsw-4770:        [INCOMPLETE][16] ([i915#4785]) -> [PASS][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12100/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108314v1/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-skl-6700k2:      [INCOMPLETE][18] ([i915#6598]) -> [PASS][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12100/fi-skl-6700k2/igt@kms_chamelium@common-hpd-after-suspend.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108314v1/fi-skl-6700k2/igt@kms_chamelium@common-hpd-after-suspend.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size:
    - fi-bsw-kefka:       [FAIL][20] ([i915#6298]) -> [PASS][21]
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12100/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108314v1/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#2867]: https://gitlab.freedesktop.org/drm/intel/issues/2867
  [i915#3303]: https://gitlab.freedesktop.org/drm/intel/issues/3303
  [i915#3921]: https://gitlab.freedesktop.org/drm/intel/issues/3921
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4418]: https://gitlab.freedesktop.org/drm/intel/issues/4418
  [i915#4494]: https://gitlab.freedesktop.org/drm/intel/issues/4494
  [i915#4528]: https://gitlab.freedesktop.org/drm/intel/issues/4528
  [i915#4785]: https://gitlab.freedesktop.org/drm/intel/issues/4785
  [i915#4957]: https://gitlab.freedesktop.org/drm/intel/issues/4957
  [i915#6011]: https://gitlab.freedesktop.org/drm/intel/issues/6011
  [i915#6246]: https://gitlab.freedesktop.org/drm/intel/issues/6246
  [i915#6298]: https://gitlab.freedesktop.org/drm/intel/issues/6298
  [i915#6598]: https://gitlab.freedesktop.org/drm/intel/issues/6598


Build changes
-------------

  * Linux: CI_DRM_12100 -> Patchwork_108314v1

  CI-20190529: 20190529
  CI_DRM_12100: 97f3651d0cd43a46f8065e7f64107f76bb9f704b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6648: 3c9079c0b97445fbfc903b9c5a1d69707b80af80 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_108314v1: 97f3651d0cd43a46f8065e7f64107f76bb9f704b @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

1ddcab3bf382 drm/i915: remove excessive i915_gem_drain_freed_objects
b9c37f3555af drm/i915: Fix a potential UAF at device unload

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108314v1/index.html

[-- Attachment #2: Type: text/html, Size: 8053 bytes --]

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Fix a potential UAF at device unload

[Tvrtko Ursulin]

On 08/09/2022 21:07, Nirmoy Das wrote:
> i915_gem_drain_freed_objects() might not be enough to
> free all the objects and RCU delayed work might get
> scheduled after the i915 device struct gets freed.
> 
> Call i915_gem_drain_workqueue() to catch all RCU delayed work.
> 
> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0f49ec9d494a..e8a053eaaa89 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1254,7 +1254,7 @@ void i915_gem_init_early(struct drm_i915_private *dev_priv)
>   
>   void i915_gem_cleanup_early(struct drm_i915_private *dev_priv)
>   {
> -	i915_gem_drain_freed_objects(dev_priv);
> +	i915_gem_drain_workqueue(dev_priv);
>   	GEM_BUG_ON(!llist_empty(&dev_priv->mm.free_list));
>   	GEM_BUG_ON(atomic_read(&dev_priv->mm.free_count));
>   	drm_WARN_ON(&dev_priv->drm, dev_priv->mm.shrink_count);


Help me spot the place where RCU free worker schedules itself back to 
free more objects - if I got the rationale here right?

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH 2/2] drm/i915: remove excessive i915_gem_drain_freed_objects

[Tvrtko Ursulin]

On 08/09/2022 21:07, Nirmoy Das wrote:
> i915_gem_drain_workqueue() call i915_gem_drain_freed_objects()
> so no need to call that again.
> 
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c                  | 2 --
>   drivers/gpu/drm/i915/selftests/mock_gem_device.c | 1 -
>   2 files changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e8a053eaaa89..e16718d79533 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1217,8 +1217,6 @@ void i915_gem_driver_remove(struct drm_i915_private *dev_priv)
>   
>   	/* Flush any outstanding unpin_work. */
>   	i915_gem_drain_workqueue(dev_priv);
> -
> -	i915_gem_drain_freed_objects(dev_priv);
>   }
>   
>   void i915_gem_driver_release(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> index f5904e659ef2..5d02346c43a2 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> @@ -67,7 +67,6 @@ static void mock_device_release(struct drm_device *dev)
>   	intel_gt_driver_remove(to_gt(i915));
>   
>   	i915_gem_drain_workqueue(i915);
> -	i915_gem_drain_freed_objects(i915);
>   
>   	mock_fini_ggtt(to_gt(i915)->ggtt);
>   	destroy_workqueue(i915->wq);

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Fix a potential UAF at device unload

[Das, Nirmoy]

On 9/9/2022 10:55 AM, Tvrtko Ursulin wrote:
>
> On 08/09/2022 21:07, Nirmoy Das wrote:
>> i915_gem_drain_freed_objects() might not be enough to
>> free all the objects and RCU delayed work might get
>> scheduled after the i915 device struct gets freed.
>>
>> Call i915_gem_drain_workqueue() to catch all RCU delayed work.
>>
>> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c 
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index 0f49ec9d494a..e8a053eaaa89 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -1254,7 +1254,7 @@ void i915_gem_init_early(struct 
>> drm_i915_private *dev_priv)
>>     void i915_gem_cleanup_early(struct drm_i915_private *dev_priv)
>>   {
>> -    i915_gem_drain_freed_objects(dev_priv);
>> +    i915_gem_drain_workqueue(dev_priv);
>>       GEM_BUG_ON(!llist_empty(&dev_priv->mm.free_list));
>>       GEM_BUG_ON(atomic_read(&dev_priv->mm.free_count));
>>       drm_WARN_ON(&dev_priv->drm, dev_priv->mm.shrink_count);
>
>
> Help me spot the place where RCU free worker schedules itself back to 
> free more objects - if I got the rationale here right?
(Sorry for late reply, was on leave last week.)

I had to clarify this with Chris. So when driver frees a obj, it does 
dma_resv_fini() which will drop reference

for all the fences in it and a fence might  reference to an object and 
upon release of that fence can trigger a  release reference to an object.

Regards,

Nirmoy



>
> Regards,
>
> Tvrtko

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Fix a potential UAF at device unload

[Tvrtko Ursulin]

On 21/09/2022 16:53, Das, Nirmoy wrote:
> 
> On 9/9/2022 10:55 AM, Tvrtko Ursulin wrote:
>>
>> On 08/09/2022 21:07, Nirmoy Das wrote:
>>> i915_gem_drain_freed_objects() might not be enough to
>>> free all the objects and RCU delayed work might get
>>> scheduled after the i915 device struct gets freed.
>>>
>>> Call i915_gem_drain_workqueue() to catch all RCU delayed work.
>>>
>>> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_gem.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c 
>>> b/drivers/gpu/drm/i915/i915_gem.c
>>> index 0f49ec9d494a..e8a053eaaa89 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -1254,7 +1254,7 @@ void i915_gem_init_early(struct 
>>> drm_i915_private *dev_priv)
>>>     void i915_gem_cleanup_early(struct drm_i915_private *dev_priv)
>>>   {
>>> -    i915_gem_drain_freed_objects(dev_priv);
>>> +    i915_gem_drain_workqueue(dev_priv);
>>>       GEM_BUG_ON(!llist_empty(&dev_priv->mm.free_list));
>>>       GEM_BUG_ON(atomic_read(&dev_priv->mm.free_count));
>>>       drm_WARN_ON(&dev_priv->drm, dev_priv->mm.shrink_count);
>>
>>
>> Help me spot the place where RCU free worker schedules itself back to 
>> free more objects - if I got the rationale here right?
> (Sorry for late reply, was on leave last week.)
> 
> I had to clarify this with Chris. So when driver frees a obj, it does 
> dma_resv_fini() which will drop reference
> 
> for all the fences in it and a fence might  reference to an object and 
> upon release of that fence can trigger a  release reference to an object.

Hmm I couldn't find that in code but never mind. It's just a stronger 
version of the same flushing and it's not on a path where speed matters 
so feel free to go with it.

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Fix a potential UAF at device unload

[Das, Nirmoy]

On 9/22/2022 11:37 AM, Tvrtko Ursulin wrote:
>
> On 21/09/2022 16:53, Das, Nirmoy wrote:
>>
>> On 9/9/2022 10:55 AM, Tvrtko Ursulin wrote:
>>>
>>> On 08/09/2022 21:07, Nirmoy Das wrote:
>>>> i915_gem_drain_freed_objects() might not be enough to
>>>> free all the objects and RCU delayed work might get
>>>> scheduled after the i915 device struct gets freed.
>>>>
>>>> Call i915_gem_drain_workqueue() to catch all RCU delayed work.
>>>>
>>>> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_gem.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c 
>>>> b/drivers/gpu/drm/i915/i915_gem.c
>>>> index 0f49ec9d494a..e8a053eaaa89 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>> @@ -1254,7 +1254,7 @@ void i915_gem_init_early(struct 
>>>> drm_i915_private *dev_priv)
>>>>     void i915_gem_cleanup_early(struct drm_i915_private *dev_priv)
>>>>   {
>>>> -    i915_gem_drain_freed_objects(dev_priv);
>>>> +    i915_gem_drain_workqueue(dev_priv);
>>>> GEM_BUG_ON(!llist_empty(&dev_priv->mm.free_list));
>>>> GEM_BUG_ON(atomic_read(&dev_priv->mm.free_count));
>>>>       drm_WARN_ON(&dev_priv->drm, dev_priv->mm.shrink_count);
>>>
>>>
>>> Help me spot the place where RCU free worker schedules itself back 
>>> to free more objects - if I got the rationale here right?
>> (Sorry for late reply, was on leave last week.)
>>
>> I had to clarify this with Chris. So when driver frees a obj, it does 
>> dma_resv_fini() which will drop reference
>>
>> for all the fences in it and a fence might  reference to an object 
>> and upon release of that fence can trigger a  release reference to an 
>> object.
>
> Hmm I couldn't find that in code but never mind. It's just a stronger 
> version of the same flushing and it's not on a path where speed 
> matters so feel free to go with it.


Can I get a Ack from you for this, Tvrtko ?


Thanks,

Nirmoy

>
> Regards,
>
> Tvrtko

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Fix a potential UAF at device unload

[Tvrtko Ursulin]

On 22/09/2022 13:11, Das, Nirmoy wrote:
> 
> On 9/22/2022 11:37 AM, Tvrtko Ursulin wrote:
>>
>> On 21/09/2022 16:53, Das, Nirmoy wrote:
>>>
>>> On 9/9/2022 10:55 AM, Tvrtko Ursulin wrote:
>>>>
>>>> On 08/09/2022 21:07, Nirmoy Das wrote:
>>>>> i915_gem_drain_freed_objects() might not be enough to
>>>>> free all the objects and RCU delayed work might get
>>>>> scheduled after the i915 device struct gets freed.
>>>>>
>>>>> Call i915_gem_drain_workqueue() to catch all RCU delayed work.
>>>>>
>>>>> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>>>> ---
>>>>>   drivers/gpu/drm/i915/i915_gem.c | 2 +-
>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c 
>>>>> b/drivers/gpu/drm/i915/i915_gem.c
>>>>> index 0f49ec9d494a..e8a053eaaa89 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>>> @@ -1254,7 +1254,7 @@ void i915_gem_init_early(struct 
>>>>> drm_i915_private *dev_priv)
>>>>>     void i915_gem_cleanup_early(struct drm_i915_private *dev_priv)
>>>>>   {
>>>>> -    i915_gem_drain_freed_objects(dev_priv);
>>>>> +    i915_gem_drain_workqueue(dev_priv);
>>>>> GEM_BUG_ON(!llist_empty(&dev_priv->mm.free_list));
>>>>> GEM_BUG_ON(atomic_read(&dev_priv->mm.free_count));
>>>>>       drm_WARN_ON(&dev_priv->drm, dev_priv->mm.shrink_count);
>>>>
>>>>
>>>> Help me spot the place where RCU free worker schedules itself back 
>>>> to free more objects - if I got the rationale here right?
>>> (Sorry for late reply, was on leave last week.)
>>>
>>> I had to clarify this with Chris. So when driver frees a obj, it does 
>>> dma_resv_fini() which will drop reference
>>>
>>> for all the fences in it and a fence might  reference to an object 
>>> and upon release of that fence can trigger a  release reference to an 
>>> object.
>>
>> Hmm I couldn't find that in code but never mind. It's just a stronger 
>> version of the same flushing and it's not on a path where speed 
>> matters so feel free to go with it.
> 
> 
> Can I get a Ack from you for this, Tvrtko ?

Sorry yes, forgot to be explicit.

Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Fix a potential UAF at device unload

[Das, Nirmoy]

On 9/22/2022 2:28 PM, Tvrtko Ursulin wrote:
>
> On 22/09/2022 13:11, Das, Nirmoy wrote:
>>
>> On 9/22/2022 11:37 AM, Tvrtko Ursulin wrote:
>>>
>>> On 21/09/2022 16:53, Das, Nirmoy wrote:
>>>>
>>>> On 9/9/2022 10:55 AM, Tvrtko Ursulin wrote:
>>>>>
>>>>> On 08/09/2022 21:07, Nirmoy Das wrote:
>>>>>> i915_gem_drain_freed_objects() might not be enough to
>>>>>> free all the objects and RCU delayed work might get
>>>>>> scheduled after the i915 device struct gets freed.
>>>>>>
>>>>>> Call i915_gem_drain_workqueue() to catch all RCU delayed work.
>>>>>>
>>>>>> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/i915/i915_gem.c | 2 +-
>>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c 
>>>>>> b/drivers/gpu/drm/i915/i915_gem.c
>>>>>> index 0f49ec9d494a..e8a053eaaa89 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>>>> @@ -1254,7 +1254,7 @@ void i915_gem_init_early(struct 
>>>>>> drm_i915_private *dev_priv)
>>>>>>     void i915_gem_cleanup_early(struct drm_i915_private *dev_priv)
>>>>>>   {
>>>>>> -    i915_gem_drain_freed_objects(dev_priv);
>>>>>> +    i915_gem_drain_workqueue(dev_priv);
>>>>>> GEM_BUG_ON(!llist_empty(&dev_priv->mm.free_list));
>>>>>> GEM_BUG_ON(atomic_read(&dev_priv->mm.free_count));
>>>>>>       drm_WARN_ON(&dev_priv->drm, dev_priv->mm.shrink_count);
>>>>>
>>>>>
>>>>> Help me spot the place where RCU free worker schedules itself back 
>>>>> to free more objects - if I got the rationale here right?
>>>> (Sorry for late reply, was on leave last week.)
>>>>
>>>> I had to clarify this with Chris. So when driver frees a obj, it 
>>>> does dma_resv_fini() which will drop reference
>>>>
>>>> for all the fences in it and a fence might  reference to an object 
>>>> and upon release of that fence can trigger a  release reference to 
>>>> an object.
>>>
>>> Hmm I couldn't find that in code but never mind. It's just a 
>>> stronger version of the same flushing and it's not on a path where 
>>> speed matters so feel free to go with it.
>>
>>
>> Can I get a Ack from you for this, Tvrtko ?
>
> Sorry yes, forgot to be explicit.
>
> Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>


Thanks a lot. I will rebase and send again.


Nirmoy

>
> Regards,
>
> Tvrtko

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Fix a potential UAF at device unload

[Das, Nirmoy]

Hi Ville,

On 11/2/2022 6:55 PM, Ville Syrjälä wrote:
> On Mon, Oct 24, 2022 at 10:08:29AM +0200, Das, Nirmoy wrote:
>> On 10/21/2022 6:34 PM, Ville Syrjälä wrote:
>>> On Fri, Sep 23, 2022 at 09:35:14AM +0200, Nirmoy Das wrote:
>>>> i915_gem_drain_freed_objects() might not be enough to
>>>> free all the objects and RCU delayed work might get
>>>> scheduled after the i915 device struct gets freed.
>>>>
>>>> Call i915_gem_drain_workqueue() to catch all RCU delayed work.
>>> shard-snb is stil hitting the mm.shrink_count WARNn reliably,
>>> and things go downhill after that.
>>
>> Looks better now again. Going to look into that.
> Looks to be still hitting it occasionally in module reload tests:
> https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7033/shard-snb5/igt@i915_module_load@reload.html
> https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7035/shard-snb7/igt@perf_pmu@module-unload.html


There are no snb in RIl so I ran this test on tgl-u for 6+ hours without 
any reproduction. Not sure why snb is so special here.

May be we need your previous patch as well ? I will be on vacation from 
next week so unfortunately I won't be able work on it for few  weeks.


Regards,

Nirmoy

>
>>
>> Thanks,
>>
>> Nirmoy
>>
>>>> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
>>>> Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_gem.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>>> index 88df9a35e0fe..7541028caebd 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>> @@ -1278,7 +1278,7 @@ void i915_gem_init_early(struct drm_i915_private *dev_priv)
>>>>    
>>>>    void i915_gem_cleanup_early(struct drm_i915_private *dev_priv)
>>>>    {
>>>> -	i915_gem_drain_freed_objects(dev_priv);
>>>> +	i915_gem_drain_workqueue(dev_priv);
>>>>    	GEM_BUG_ON(!llist_empty(&dev_priv->mm.free_list));
>>>>    	GEM_BUG_ON(atomic_read(&dev_priv->mm.free_count));
>>>>    	drm_WARN_ON(&dev_priv->drm, dev_priv->mm.shrink_count);
>>>> -- 
>>>> 2.37.3

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Fix a potential UAF at device unload

[Ville Syrjälä]

On Mon, Oct 24, 2022 at 10:08:29AM +0200, Das, Nirmoy wrote:
> 
> On 10/21/2022 6:34 PM, Ville Syrjälä wrote:
> > On Fri, Sep 23, 2022 at 09:35:14AM +0200, Nirmoy Das wrote:
> >> i915_gem_drain_freed_objects() might not be enough to
> >> free all the objects and RCU delayed work might get
> >> scheduled after the i915 device struct gets freed.
> >>
> >> Call i915_gem_drain_workqueue() to catch all RCU delayed work.
> > shard-snb is stil hitting the mm.shrink_count WARNn reliably,
> > and things go downhill after that.
> 
> 
> Looks better now again. Going to look into that.

Looks to be still hitting it occasionally in module reload tests:
https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7033/shard-snb5/igt@i915_module_load@reload.html
https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7035/shard-snb7/igt@perf_pmu@module-unload.html

> 
> 
> Thanks,
> 
> Nirmoy
> 
> >
> >> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
> >> Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/i915_gem.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >> index 88df9a35e0fe..7541028caebd 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem.c
> >> @@ -1278,7 +1278,7 @@ void i915_gem_init_early(struct drm_i915_private *dev_priv)
> >>   
> >>   void i915_gem_cleanup_early(struct drm_i915_private *dev_priv)
> >>   {
> >> -	i915_gem_drain_freed_objects(dev_priv);
> >> +	i915_gem_drain_workqueue(dev_priv);
> >>   	GEM_BUG_ON(!llist_empty(&dev_priv->mm.free_list));
> >>   	GEM_BUG_ON(atomic_read(&dev_priv->mm.free_count));
> >>   	drm_WARN_ON(&dev_priv->drm, dev_priv->mm.shrink_count);
> >> -- 
> >> 2.37.3

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Fix a potential UAF at device unload

[Das, Nirmoy]

On 10/21/2022 6:34 PM, Ville Syrjälä wrote:
> On Fri, Sep 23, 2022 at 09:35:14AM +0200, Nirmoy Das wrote:
>> i915_gem_drain_freed_objects() might not be enough to
>> free all the objects and RCU delayed work might get
>> scheduled after the i915 device struct gets freed.
>>
>> Call i915_gem_drain_workqueue() to catch all RCU delayed work.
> shard-snb is stil hitting the mm.shrink_count WARNn reliably,
> and things go downhill after that.


Looks better now again. Going to look into that.


Thanks,

Nirmoy

>
>> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
>> Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 88df9a35e0fe..7541028caebd 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -1278,7 +1278,7 @@ void i915_gem_init_early(struct drm_i915_private *dev_priv)
>>   
>>   void i915_gem_cleanup_early(struct drm_i915_private *dev_priv)
>>   {
>> -	i915_gem_drain_freed_objects(dev_priv);
>> +	i915_gem_drain_workqueue(dev_priv);
>>   	GEM_BUG_ON(!llist_empty(&dev_priv->mm.free_list));
>>   	GEM_BUG_ON(atomic_read(&dev_priv->mm.free_count));
>>   	drm_WARN_ON(&dev_priv->drm, dev_priv->mm.shrink_count);
>> -- 
>> 2.37.3

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Fix a potential UAF at device unload

[Ville Syrjälä]

On Fri, Sep 23, 2022 at 09:35:14AM +0200, Nirmoy Das wrote:
> i915_gem_drain_freed_objects() might not be enough to
> free all the objects and RCU delayed work might get
> scheduled after the i915 device struct gets freed.
> 
> Call i915_gem_drain_workqueue() to catch all RCU delayed work.

shard-snb is stil hitting the mm.shrink_count WARNn reliably,
and things go downhill after that.

> 
> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
> Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 88df9a35e0fe..7541028caebd 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1278,7 +1278,7 @@ void i915_gem_init_early(struct drm_i915_private *dev_priv)
>  
>  void i915_gem_cleanup_early(struct drm_i915_private *dev_priv)
>  {
> -	i915_gem_drain_freed_objects(dev_priv);
> +	i915_gem_drain_workqueue(dev_priv);
>  	GEM_BUG_ON(!llist_empty(&dev_priv->mm.free_list));
>  	GEM_BUG_ON(atomic_read(&dev_priv->mm.free_count));
>  	drm_WARN_ON(&dev_priv->drm, dev_priv->mm.shrink_count);
> -- 
> 2.37.3

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Fix a potential UAF at device unload

[Das, Nirmoy]

On 9/29/2022 1:32 PM, Andi Shyti wrote:
> Hi Nirmoy,
>
> On Fri, Sep 23, 2022 at 09:35:14AM +0200, Nirmoy Das wrote:
>> i915_gem_drain_freed_objects() might not be enough to
>> free all the objects and RCU delayed work might get
>> scheduled after the i915 device struct gets freed.
>>
>> Call i915_gem_drain_workqueue() to catch all RCU delayed work.
>>
>> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
>> Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> pushed to drm-intel-gt-next


Thanks, Andi!

>
> Thanks,
> Andi

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Fix a potential UAF at device unload

[Andi Shyti]

Hi Nirmoy,

On Fri, Sep 23, 2022 at 09:35:14AM +0200, Nirmoy Das wrote:
> i915_gem_drain_freed_objects() might not be enough to
> free all the objects and RCU delayed work might get
> scheduled after the i915 device struct gets freed.
> 
> Call i915_gem_drain_workqueue() to catch all RCU delayed work.
> 
> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
> Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>

pushed to drm-intel-gt-next

Thanks,
Andi

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Fix a potential UAF at device unload

[Andrzej Hajda]

On 23.09.2022 09:35, Nirmoy Das wrote:
> i915_gem_drain_freed_objects() might not be enough to
> free all the objects and RCU delayed work might get
> scheduled after the i915 device struct gets freed.
> 
> Call i915_gem_drain_workqueue() to catch all RCU delayed work.
> 
> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
> Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>

Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej

> ---
>   drivers/gpu/drm/i915/i915_gem.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 88df9a35e0fe..7541028caebd 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1278,7 +1278,7 @@ void i915_gem_init_early(struct drm_i915_private *dev_priv)
>   
>   void i915_gem_cleanup_early(struct drm_i915_private *dev_priv)
>   {
> -	i915_gem_drain_freed_objects(dev_priv);
> +	i915_gem_drain_workqueue(dev_priv);
>   	GEM_BUG_ON(!llist_empty(&dev_priv->mm.free_list));
>   	GEM_BUG_ON(atomic_read(&dev_priv->mm.free_count));
>   	drm_WARN_ON(&dev_priv->drm, dev_priv->mm.shrink_count);

[Intel-gfx] [PATCH 1/2] drm/i915: Fix a potential UAF at device unload

[Nirmoy Das]

i915_gem_drain_freed_objects() might not be enough to
free all the objects and RCU delayed work might get
scheduled after the i915 device struct gets freed.

Call i915_gem_drain_workqueue() to catch all RCU delayed work.

Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 88df9a35e0fe..7541028caebd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1278,7 +1278,7 @@ void i915_gem_init_early(struct drm_i915_private *dev_priv)
 
 void i915_gem_cleanup_early(struct drm_i915_private *dev_priv)
 {
-	i915_gem_drain_freed_objects(dev_priv);
+	i915_gem_drain_workqueue(dev_priv);
 	GEM_BUG_ON(!llist_empty(&dev_priv->mm.free_list));
 	GEM_BUG_ON(atomic_read(&dev_priv->mm.free_count));
 	drm_WARN_ON(&dev_priv->drm, dev_priv->mm.shrink_count);
-- 
2.37.3