linux-kernel - Re: [PATCH] drm/i915/gt: Handle INTEL_WAKEREF_DEF return value in gen8_ggtt_bind_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87cyglg9w2.fsf@intel.com>
Date: Fri, 17 Jan 2025 09:46:53 +0200
From: Jani Nikula <jani.nikula@...ux.intel.com>
To: Ingyu Jang <ingyujang25@...il.com>, Joonas Lahtinen
 <joonas.lahtinen@...ux.intel.com>, Rodrigo Vivi <rodrigo.vivi@...el.com>,
 Tvrtko Ursulin <tursulin@...ulin.net>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>
Cc: Ingyu Jang <ingyujang25@...il.com>, intel-gfx@...ts.freedesktop.org,
 dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/i915/gt: Handle INTEL_WAKEREF_DEF return value in
 gen8_ggtt_bind_get_ce

On Fri, 17 Jan 2025, Ingyu Jang <ingyujang25@...il.com> wrote:
> The function 'gen8_ggtt_bind_get_ce' previously did not handle the case
> where 'intel_gt_pm_get_if_awake()' returns 'INTEL_WAKEREF_DEF'.
> This value is returned when the 'intel_ref_tracker_alloc()' call within
> 'intel_gt_pm_get_if_awake()' fails to allocate due
> to memory pressure or other constraints.
>
> 'intel_ref_tracker_alloc()' uses 'ref_tracker_alloc()' with the
> 'GFP_NOWAIT' flag, which can return 'NULL' if allocation fails.
> In this case, the function explicitly returns 'INTEL_WAKEREF_DEF'.
>
> This patch adds a check for 'INTEL_WAKEREF_DEF' in
> 'gen8_ggtt_bind_get_ce' to ensure that such errors are properly handled.
> If 'INTEL_WAKEREF_DEF' is returned, the function returns 'NULL' .

No.

The callers should only handle NULL vs. non-NULL, and never make any
other assumptions about the value.

If intel_ref_tracker_alloc() fails, we'll only lose the wakeref tracking
debug aid, but everything else goes fine. The patch at hand conflates
the returned "asleep" (NULL) with "ref tracker fail", and that's wrong.

See how intel_ref_tracker_free() handles INTEL_WAKEREF_DEF.

BR,
Jani.


>
> Signed-off-by: Ingyu Jang <ingyujang25@...il.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_ggtt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> index d60a6ca0cae5..8d22d8f2243d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> @@ -311,7 +311,7 @@ static struct intel_context *gen8_ggtt_bind_get_ce(struct i915_ggtt *ggtt, intel
>  	 * doing rpm_resume().
>  	 */
>  	*wakeref = intel_gt_pm_get_if_awake(gt);
> -	if (!*wakeref)
> +	if (!*wakeref || *wakeref == INTEL_WAKEREF_DEF)
>  		return NULL;
>  
>  	intel_engine_pm_get(ce->engine);

-- 
Jani Nikula, Intel