Hi guys, well going back to the beginning once more because something doesn't fit together here.
I mean what exactly is the purpose of 8059add0478e "drm: allow render capable master with DRM_AUTH ioctls"? When I read the code correctly the effect is that we ignore the DRM_AUTH flag when also the DRM_RENDER_ALLOW flag is set and the driver is a render node driver. Well when this is correct then why the heck aren't we removing the DRM_AUTH flag from the affected IOCTLs instead? The only common IOCTLs affecting this are DRM_IOCTL_PRIME_HANDLE_TO_FD and DRM_IOCTL_PRIME_FD_TO_HANDLE and those are protected by capability flags separately. So the patch 8059add0478e doesn't make any sense as far as I can tell. What I'm missing? Regards, Christian. Am 17.04.19 um 11:18 schrieb Christian König: > Am 17.04.19 um 10:10 schrieb Daniel Vetter: >> On Wed, Apr 17, 2019 at 09:09:27AM +0200, Christian König wrote: >>> Am 16.04.19 um 23:40 schrieb Andres Rodriguez: >>>> After a recent commit, access to the DRM_AUTH ioctls become more >>>> permissive. This resulted in a buggy check for drm_master capabilities >>>> inside radv stop working. >>>> >>>> This commit adds a backwards compatibility workaround so that the radv >>>> drm_master check keeps working as previously expected. >>>> >>>> This fixes SteamVR and other drm lease based apps being broken since >>>> v5.1-rc1 >>>> >>>> From the usermode side, radv will also be fixed to properly test for >>>> master capabilities: >>>> https://gitlab.freedesktop.org/mesa/mesa/merge_requests/669 >>>> >>>> Fixes: 8059add0478e ("drm: allow render capable master with >>>> DRM_AUTH ioctls") >>>> Signed-off-by: Andres Rodriguez <andre...@gmail.com> >>>> Cc: Daniel Vetter <dan...@ffwll.ch> >>>> Cc: David Airlie <airl...@linux.ie> >>>> Cc: Emil Velikov <emil.veli...@collabora.com> >>>> Cc: Alex Deucher <alexander.deuc...@amd.com> >>>> Cc: Keith Packard <kei...@keithp.com> >>>> Reviewed-by: Keith Packard <kei...@keithp.com> >>>> Reviewed-by: Daniel Vetter <dan...@ffwll.ch> >>> Well definitely a NAK. IIRC we have unit tests where the exactly >>> first thing >>> they do is querying AMDGPU_INFO_ACCEL_WORKING. >>> >>> And I definitely not going to risk breaking those just to fix buggy >>> behavior >>> in radv. >> s/buggy/fragile :-) >> >> Option B would be to disable 8059add0478e ("drm: allow render capable >> master with DRM_AUTH ioctls") just for amdgpu.ko, but that's a bit more >> tricky to implement I guess (since it'd leak all over the place). >> >> Option C is to revert 8059add04, but that's a bit silly since it >> seems to >> work everywhere. >> >> Breaking radv isn't an option, because no regression. Aside: No one is >> stopping amd folks from reviewing radv patches and making sure >> there's no >> fragile stuff in there. >> >> We discussed this quite a bit on irc with Ben and Keith and others, and >> figured option A is the most promising to go forward with. Anything >> using >> amdgpu_device_init (which I think are all the umds, but I didn't check) >> will keep working, as will radv leases/vkdisplay, plus we can keep >> 8059add04 for everyone (not just everony except amdgpu). If that means >> breaking a few unit tests ... *shrugh*. Needs patches to fix them >> ofc, but >> shouldn't be that much work really. > > I think you miss the point here: This patch will break the render node > interface! > > E.g. take another look at those lines of code: >> + if (fpriv->is_first_ioctl && !filp->is_master) >> + return -EACCES; > This will return -EACCES on the first accel working query which is > doesn't comes from the DRM master. And it is irrelevant if its the > primary or the render node. > > So this patch most likely breaks tons of things and is *definitely* a > complete NAK. > > Additional to that IIRC we have (rather old) code which behaves > differently depending if it is called by the render node or the > primary node. In other words it assumes that the primary node is > authenticated. > > If I understood correctly what 8059add04 does than this is NOT a good > to do in general. > > Regards, > Christian. > >> -Daniel >> >>> Christian. >>> >>>> --- >>>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 3 +++ >>>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 ++ >>>> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 22 ++++++++++++++++++++++ >>>> 3 files changed, 27 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>> index 8d0d7f3dd5fb..e67bfee8d411 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>> @@ -409,6 +409,9 @@ struct amdgpu_fpriv { >>>> struct mutex bo_list_lock; >>>> struct idr bo_list_handles; >>>> struct amdgpu_ctx_mgr ctx_mgr; >>>> + >>>> + /* Part of a backwards compatibility hack */ >>>> + bool is_first_ioctl; >>>> }; >>>> int amdgpu_file_to_fpriv(struct file *filp, struct amdgpu_fpriv >>>> **fpriv); >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c >>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c >>>> index 7419ea8a388b..cd438afa7092 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c >>>> @@ -1128,6 +1128,7 @@ long amdgpu_drm_ioctl(struct file *filp, >>>> unsigned int cmd, unsigned long arg) >>>> { >>>> struct drm_file *file_priv = filp->private_data; >>>> + struct amdgpu_fpriv *fpriv = file_priv->driver_priv; >>>> struct drm_device *dev; >>>> long ret; >>>> dev = file_priv->minor->dev; >>>> @@ -1136,6 +1137,7 @@ long amdgpu_drm_ioctl(struct file *filp, >>>> return ret; >>>> ret = drm_ioctl(filp, cmd, arg); >>>> + fpriv->is_first_ioctl = false; >>>> pm_runtime_mark_last_busy(dev->dev); >>>> pm_runtime_put_autosuspend(dev->dev); >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c >>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c >>>> index e860412043bb..00c0a2fb2a69 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c >>>> @@ -455,6 +455,7 @@ static int amdgpu_hw_ip_info(struct >>>> amdgpu_device *adev, >>>> static int amdgpu_info_ioctl(struct drm_device *dev, void *data, >>>> struct drm_file *filp) >>>> { >>>> struct amdgpu_device *adev = dev->dev_private; >>>> + struct amdgpu_fpriv *fpriv = filp->driver_priv; >>>> struct drm_amdgpu_info *info = data; >>>> struct amdgpu_mode_info *minfo = &adev->mode_info; >>>> void __user *out = (void __user >>>> *)(uintptr_t)info->return_pointer; >>>> @@ -470,6 +471,26 @@ static int amdgpu_info_ioctl(struct drm_device >>>> *dev, void *data, struct drm_file >>>> switch (info->query) { >>>> case AMDGPU_INFO_ACCEL_WORKING: >>>> + /* HACK: radv has a fragile open-coded check for drm master >>>> + * The pattern for the call is: >>>> + * open(DRM_NODE_PRIMARY) >>>> + * drmModeCommandWrite( query=AMDGPU_INFO_ACCEL_WORKING ) >>>> + * >>>> + * After commit 8059add04 this check stopped working due to >>>> + * operations on a primary node from non-master clients >>>> becoming >>>> + * more permissive. >>>> + * >>>> + * This backwards compatibility hack targets the calling >>>> pattern >>>> + * above to fail radv from thinking it has master access >>>> to the >>>> + * display system ( without reverting 8059add04 ). >>>> + * >>>> + * Users of libdrm will not be affected as some other >>>> ioctls are >>>> + * performed between open() and the AMDGPU_INFO_ACCEL_WORKING >>>> + * query. >>>> + */ >>>> + if (fpriv->is_first_ioctl && !filp->is_master) >>>> + return -EACCES; >>>> + >>>> ui32 = adev->accel_working; >>>> return copy_to_user(out, &ui32, min(size, 4u)) ? -EFAULT >>>> : 0; >>>> case AMDGPU_INFO_CRTC_FROM_ID: >>>> @@ -987,6 +1008,7 @@ int amdgpu_driver_open_kms(struct drm_device >>>> *dev, struct drm_file *file_priv) >>>> goto error_vm; >>>> } >>>> + fpriv->is_first_ioctl = true; >>>> mutex_init(&fpriv->bo_list_lock); >>>> idr_init(&fpriv->bo_list_handles); >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
