On Thu, Aug 8, 2024 at 1:48 PM Victor Skvortsov
<victor.skvortsov@xxxxxxx> wrote:
>
> Register access from userspace should be blocked until
> reset is complete.
While you are at it, you should protect AMDGPU_INFO_SENSOR as well.
That queries PMFW which doesn't make sense during reset. A few minor
comments below as well.
>
> Signed-off-by: Victor Skvortsov <victor.skvortsov@xxxxxxx>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 44 ++++++++++++++++++-------
> 1 file changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 260cd0ad286d..038b400be437 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -778,6 +778,7 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
> ? -EFAULT : 0;
> }
> case AMDGPU_INFO_READ_MMR_REG: {
> + int ret = 0;
> unsigned int n, alloc_size;
> uint32_t *regs;
> unsigned int se_num = (info->read_mmr_reg.instance >>
> @@ -787,24 +788,39 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
> AMDGPU_INFO_MMR_SH_INDEX_SHIFT) &
> AMDGPU_INFO_MMR_SH_INDEX_MASK;
>
> + if (!down_read_trylock(&adev->reset_domain->sem))
> + return -ENOENT;
> +
> /* set full masks if the userspace set all bits
> * in the bitfields
> */
> - if (se_num == AMDGPU_INFO_MMR_SE_INDEX_MASK)
> + if (se_num == AMDGPU_INFO_MMR_SE_INDEX_MASK) {
> se_num = 0xffffffff;
> - else if (se_num >= AMDGPU_GFX_MAX_SE)
> - return -EINVAL;
> - if (sh_num == AMDGPU_INFO_MMR_SH_INDEX_MASK)
> + }
> + else if (se_num >= AMDGPU_GFX_MAX_SE) {
Kernel coding style. Put the } on the same line as the else.
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (sh_num == AMDGPU_INFO_MMR_SH_INDEX_MASK) {
> sh_num = 0xffffffff;
> - else if (sh_num >= AMDGPU_GFX_MAX_SH_PER_SE)
> - return -EINVAL;
> + }
> + else if (sh_num >= AMDGPU_GFX_MAX_SH_PER_SE) {
Same here.
> + ret = -EINVAL;
> + goto out;
> + }
>
> - if (info->read_mmr_reg.count > 128)
> - return -EINVAL;
> + if (info->read_mmr_reg.count > 128) {
> + ret = -EINVAL;
> + goto out;
> + }
>
> regs = kmalloc_array(info->read_mmr_reg.count, sizeof(*regs), GFP_KERNEL);
> - if (!regs)
> - return -ENOMEM;
> + if (!regs) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> alloc_size = info->read_mmr_reg.count * sizeof(*regs);
>
> amdgpu_gfx_off_ctrl(adev, false);
> @@ -816,13 +832,17 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
> info->read_mmr_reg.dword_offset + i);
> kfree(regs);
> amdgpu_gfx_off_ctrl(adev, true);
> - return -EFAULT;
> + ret = -EFAULT;
> + goto out;
> }
> }
> amdgpu_gfx_off_ctrl(adev, true);
> n = copy_to_user(out, regs, min(size, alloc_size));
> kfree(regs);
> - return n ? -EFAULT : 0;
> + ret = (n ? -EFAULT : 0);
> +out:
> + up_read(&adev->reset_domain->sem);
> + return ret;
> }
> case AMDGPU_INFO_DEV_INFO: {
> struct drm_amdgpu_info_device *dev_info;
> --
> 2.34.1
>
