Re: [PATCH] drm/amdgpu: fix NULL RAS context dereference in UMC page retirement

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] amdgpu: KFD queue permanently hung when RAS is disabled due to bypassed GPU reset
--

commit 8cb95b8549bf147ff94f08bcdd4e5a3aaa6812ae
Author: Haotian Zhang <vulab@iscas.ac.cn>

drm/amdgpu: fix NULL RAS context dereference in UMC page retirement

amdgpu_umc_handle_bad_pages() and amdgpu_umc_do_page_retirement() take the
RAS context from amdgpu_ras_get_context() and dereference it without
checking for NULL.  That context is NULL whenever RAS is disabled.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> index 34702e5cb1b6c..283180e3f2db2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
[ ... ]
> @@ -217,6 +221,9 @@ static int amdgpu_umc_do_page_retirement(struct amdgpu_device *adev,
>  	kgd2kfd_set_sram_ecc_flag(adev->kfd.dev);
>  	amdgpu_umc_handle_bad_pages(adev, ras_error_status);
>  
> +	if (!con)
> +		return AMDGPU_RAS_SUCCESS;
> +

[Severity: High]
Does this early return ignore the reset parameter requested by the caller?

When KFD encounters a poison consumption error and requests a fallback GPU
reset by passing a non-zero reset flag, returning AMDGPU_RAS_SUCCESS here
without handling the reset leaves the KFD queue permanently hung.

This leaves the hardware in an unrecoverable state until the next system
reboot.

Should this trigger a generic GPU reset via amdgpu_device_gpu_recover()
when the context is NULL?

Also, is there a similar regression in amdgpu_umc_pasid_poison_handler()?

	if (adev->gmc.xgmi.connected_to_cpu ||
		adev->gmc.is_app_apu) {
		if (reset) {
			/* MCA poison handler is only responsible for GPU reset,
			 * let MCA notifier do page retirement.
			 */
			kgd2kfd_set_sram_ecc_flag(adev->kfd.dev);
			amdgpu_ras_reset_gpu(adev);
		}
		return ret;
	}

If reset is requested and xgmi.connected_to_cpu is true, this unconditionally
calls amdgpu_ras_reset_gpu().

Can this still trigger a NULL pointer dereference inside amdgpu_ras_reset_gpu()
when RAS is disabled?

>  	if ((err_data->ue_count || err_data->de_count) &&
>  	    (reset || amdgpu_ras_is_rma(adev))) {
>  		con->gpu_reset_flags |= reset;
>  		amdgpu_ras_reset_gpu(adev);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008161144.2671028-1-vulab@iscas.ac.cn?part=1