Re: [PATCH v2 1/5] drm/amdgpu: add gpu_recovery_allowed flag to amdgpu_device

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Overloading an action-triggering debugfs file with a configuration state setter creates a severe usability trap.
- [Medium] Missing initialization of `gpu_recovery_allowed` in emulation mode unintentionally disables GPU recovery.

Pre-existing issues:
- [High] Kernel stack memory leak in `gpu_recover_get` during debugfs file read.
--

--- Patch [1]: [PATCH v2 1/5] drm/amdgpu: add gpu_recovery_allowed flag to amdgpu_device ---
commit 387fc7f831c412d1422444dc6a5d877786192a5b
Author: Pierre-Eric Pelloux-Prayer <[email protected]>

drm/amdgpu: add gpu_recovery_allowed flag to amdgpu_device
    
Add a per-device boolean to control whether GPU recovery is attempted
on a hang, independently of the global amdgpu.gpu_recovery module
parameter. It defaults to true and is exposed as a write to the
existing amdgpu_gpu_recover debugfs file.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 520fd59036d57..eba75b41b50a7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4080,6 +4080,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>  		}
>  	}
>  
> +	adev->gpu_recovery_allowed = true;
> +
>  fence_driver_init:

[Severity: Medium]
Does this skip initializing the flag in emulation mode?

If amdgpu_emu_mode == 1, the code earlier in this function jumps directly to
fence_driver_init:

amdgpu_device_init() {
    if (amdgpu_emu_mode == 1) {
        emu_soc_asic_init(adev);
        goto fence_driver_init;
    }
    ...
}

Since the device structure is zero-allocated, jumping over the assignment
will leave gpu_recovery_allowed as false, which inadvertently disables GPU
recovery for this device in emulation mode. Should the initialization be
moved before the goto or handled at the beginning of the function?

[ ... ]

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> index 3043ad041bb42..707e69d8bb115 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c

[ ... ]

>  static int gpu_recover_get(void *data, u64 *val)
>  {

[Severity: High]
This is a pre-existing issue, but does this function leak kernel stack memory?

In simple_attr_read(), an uninitialized u64 val is allocated on the stack
and passed to the attribute's get callback. gpu_recover_get() returns 0
without writing to val. The uninitialized value is then formatted into a
string and copied to userspace.

Because this patch converts the file into a configuration knob, users and
tooling are much more likely to read it to check its state, making this
leak more exposed.

[ ... ]

> @@ -1001,8 +1005,17 @@ static int gpu_recover_get(void *data, u64 *val)
>  	return 0;
>  }
>  
> +static int gpu_recover_set(void *data, u64 val)
> +{
> +	struct amdgpu_device *adev = (struct amdgpu_device *)data;
> +
> +	adev->gpu_recovery_allowed = !!val;
> +
> +	return 0;
> +}
> +
>  DEFINE_SHOW_ATTRIBUTE(amdgpu_debugfs_fence_info);
> -DEFINE_DEBUGFS_ATTRIBUTE(amdgpu_debugfs_gpu_recover_fops, gpu_recover_get, NULL,
> +DEFINE_DEBUGFS_ATTRIBUTE(amdgpu_debugfs_gpu_recover_fops, gpu_recover_get, gpu_recover_set,
>  			 "%lld\n");

[Severity: High]
Does overloading this debugfs file create a usability trap?

The patch adds a set callback to configure gpu_recovery_allowed, changing
amdgpu_gpu_recover into a dual-purpose configuration file. However,
gpu_recover_get unconditionally triggers a full GPU reset when read, rather
than returning the current configuration value:

gpu_recover_get() {
    ...
    if (amdgpu_reset_domain_schedule(adev->reset_domain, &adev->reset_work))
        flush_work(&adev->reset_work);
    return 0;
}

If a user attempts to read amdgpu_gpu_recover to check if recovery is
enabled, they will unintentionally reset the GPU. Would it be safer to
use a separate debugfs file for the configuration flag?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1