[PATCH] drm/i915: Avoid refcount_inc on known zero count

[PATCH] drm/i915: Avoid refcount_inc on known zero count

[Chris Wilson]

In intel_wakeref_auto, we use refcount_inc_not_zero to detect the first
use and initialise the timer. On doing so, we have to avoid using
refcount_inc on that zero count as the debug code flags that as an
error:
	refcount_t: increment on 0; use-after-free.

Rearrange the code so that if we know the count is 0 and we are
initialising, we explicitly set it to 1.

Fixes: b27e35ae5b18 ("drm/i915: Keep user GGTT alive for a minimum of 250ms")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_wakeref.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wakeref.c b/drivers/gpu/drm/i915/intel_wakeref.c
index c2dda5a375f0..c25ba1b5e8ba 100644
--- a/drivers/gpu/drm/i915/intel_wakeref.c
+++ b/drivers/gpu/drm/i915/intel_wakeref.c
@@ -114,11 +114,11 @@ void intel_wakeref_auto(struct intel_wakeref_auto *wf, unsigned long timeout)
 
 	if (!refcount_inc_not_zero(&wf->count)) {
 		spin_lock_irqsave(&wf->lock, flags);
-		if (!refcount_read(&wf->count)) {
+		if (!refcount_inc_not_zero(&wf->count)) {
 			GEM_BUG_ON(wf->wakeref);
 			wf->wakeref = intel_runtime_pm_get_if_in_use(wf->i915);
+			refcount_set(&wf->count, 1);
 		}
-		refcount_inc(&wf->count);
 		spin_unlock_irqrestore(&wf->lock, flags);
 	}
 
-- 
2.20.1

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

✗ Fi.CI.BAT: failure for drm/i915: Avoid refcount_inc on known zero count

[Patchwork]

== Series Details ==

Series: drm/i915: Avoid refcount_inc on known zero count
URL   : https://patchwork.freedesktop.org/series/61255/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_6157 -> Patchwork_13116
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_13116 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_13116, 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_13116/

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live_sanitycheck:
    - fi-kbl-7567u:       [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6157/fi-kbl-7567u/igt@i915_selftest@live_sanitycheck.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13116/fi-kbl-7567u/igt@i915_selftest@live_sanitycheck.html

  * igt@runner@aborted:
    - fi-kbl-7567u:       NOTRUN -> [FAIL][3]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13116/fi-kbl-7567u/igt@runner@aborted.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live_contexts:
    - fi-bdw-gvtdvm:      [PASS][4] -> [DMESG-FAIL][5] ([fdo#110235])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6157/fi-bdw-gvtdvm/igt@i915_selftest@live_contexts.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13116/fi-bdw-gvtdvm/igt@i915_selftest@live_contexts.html

  * igt@i915_selftest@live_hangcheck:
    - fi-icl-u3:          [PASS][6] -> [INCOMPLETE][7] ([fdo#107713] / [fdo#108569])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6157/fi-icl-u3/igt@i915_selftest@live_hangcheck.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13116/fi-icl-u3/igt@i915_selftest@live_hangcheck.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-hsw-peppy:       [PASS][8] -> [DMESG-WARN][9] ([fdo#102614])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6157/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13116/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html

  
#### Possible fixes ####

  * igt@kms_busy@basic-flip-c:
    - fi-icl-u3:          [DMESG-WARN][10] ([fdo#107724]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6157/fi-icl-u3/igt@kms_busy@basic-flip-c.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13116/fi-icl-u3/igt@kms_busy@basic-flip-c.html

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

  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#110235]: https://bugs.freedesktop.org/show_bug.cgi?id=110235


Participating hosts (54 -> 47)
------------------------------

  Missing    (7): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-byt-clapper fi-bdw-samus 


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

  * Linux: CI_DRM_6157 -> Patchwork_13116

  CI_DRM_6157: 8991a80f85f227dc1a1ef76396ca87693ad217a6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5021: 2d64cb6808075b0d0696a89d2ce290220e6eff8e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13116: 669e72a095cee1f77d41107e282565daa0394c26 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

669e72a095ce drm/i915: Avoid refcount_inc on known zero count

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13116/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Avoid refcount_inc on known zero count

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> In intel_wakeref_auto, we use refcount_inc_not_zero to detect the first
> use and initialise the timer. On doing so, we have to avoid using
> refcount_inc on that zero count as the debug code flags that as an
> error:
> 	refcount_t: increment on 0; use-after-free.
>

Yeah there are reinforced version: refcount_inc_checked, which
I failed to notice.

I guess the good news is that now we have proof that there is
someone watching our six.

> Rearrange the code so that if we know the count is 0 and we are
> initialising, we explicitly set it to 1.
>
> Fixes: b27e35ae5b18 ("drm/i915: Keep user GGTT alive for a minimum of 250ms")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_wakeref.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_wakeref.c b/drivers/gpu/drm/i915/intel_wakeref.c
> index c2dda5a375f0..c25ba1b5e8ba 100644
> --- a/drivers/gpu/drm/i915/intel_wakeref.c
> +++ b/drivers/gpu/drm/i915/intel_wakeref.c
> @@ -114,11 +114,11 @@ void intel_wakeref_auto(struct intel_wakeref_auto *wf, unsigned long timeout)
>  
>  	if (!refcount_inc_not_zero(&wf->count)) {
>  		spin_lock_irqsave(&wf->lock, flags);
> -		if (!refcount_read(&wf->count)) {
> +		if (!refcount_inc_not_zero(&wf->count)) {

Ok, overflow is checked with this.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>


>  			GEM_BUG_ON(wf->wakeref);
>  			wf->wakeref = intel_runtime_pm_get_if_in_use(wf->i915);
> +			refcount_set(&wf->count, 1);
>  		}
> -		refcount_inc(&wf->count);
>  		spin_unlock_irqrestore(&wf->lock, flags);
>  	}
>  
> -- 
> 2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx