Re: [PATCH] drm/amdgpu: Block MMR_READ IOCTL in reset

[Christian König <christian.koenig@xxxxxxx>]

Am 08.08.24 um 19:48 schrieb Victor Skvortsov:
> Register access from userspace should be blocked until
> reset is complete.
>
> 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;

In general a good idea, but please fix the coding style. E.g. for 
example don't initialize ret here.

Regards,
Christian.

>   		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) {
> +			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) {
> +			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;