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

[Christian König]

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.skvort...@amd.com>
---
  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;