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

[Alex Deucher]

On Thu, Aug 8, 2024 at 5:33 PM Skvortsov, Victor
<Victor.Skvortsov at amd.com> wrote:
>
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> > Caution: This message originated from an External Source. Use proper
> > caution when opening attachments, clicking links, or responding.
> >
> >
> > On Thu, Aug 8, 2024 at 1:48 PM Victor Skvortsov
> > <victor.skvortsov at amd.com> 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.
>
> In general, all PMFW queries are already blocked by pm.dpm_enabled flag. This flag is toggled off in pre_asic_reset, so generally I don't think it will be a problem. Only edge cases I can think of is GFX clock gating that is setup later in hw_resume sequence. But I don't think it warrants this lock.

Fair enough.  With the coding style comments below fixed, this patch is:
Reviewed-by: Alex Deucher <alexander.deucher at amd.com>

>
> >
> > >
> > > Signed-off-by: Victor Skvortsov <victor.skvortsov at 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;
> > >                 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
> > >