| Message ID | 20191101130313.8862-5-emil.l.velikov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] drm/vmwgfx: move the require_exist handling together | expand |
On Fri, 1 Nov 2019 at 13:05, Emil Velikov <emil.l.velikov@gmail.com> wrote: > > From: Emil Velikov <emil.velikov@collabora.com> > > As mentioned by Christian, for drivers which support only primary nodes > this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS. > > For others, this check in particular will be a noop. So let's remove it > as suggested by Christian. > > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: amd-gfx@lists.freedesktop.org > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Sean Paul <sean@poorly.run> > Acked-by: Christian König <christian.koenig@amd.com> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > --- > drivers/gpu/drm/drm_ioctl.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index fcd728d7cf72..5afb39688b55 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -652,8 +652,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = { > > DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, 0), > > - DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), > - DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), > + DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW), > + DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW), > > DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, 0), > DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0), > -- Humble poke? Sean, others? Thanks Emil
Hi Emil, On Fri, 1 Nov 2019 13:03:13 +0000 Emil Velikov <emil.l.velikov@gmail.com> wrote: > From: Emil Velikov <emil.velikov@collabora.com> > > As mentioned by Christian, for drivers which support only primary nodes > this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS. Are you sure this is true for MODESET-only nodes (those that do not have the RENDER cap set) implementing ->{fd_to_handle,handle_to_fd}()? Shouldn't the is_authenticated() check still be done in that case? Regards, Boris > > For others, this check in particular will be a noop. So let's remove it > as suggested by Christian. > > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: amd-gfx@lists.freedesktop.org > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Sean Paul <sean@poorly.run> > Acked-by: Christian König <christian.koenig@amd.com> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > --- > drivers/gpu/drm/drm_ioctl.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index fcd728d7cf72..5afb39688b55 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -652,8 +652,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = { > > DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, 0), > > - DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), > - DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), > + DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW), > + DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW), > > DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, 0), > DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),
On Wed, 27 Nov 2019 at 07:41, Boris Brezillon <boris.brezillon@collabora.com> wrote: > > Hi Emil, > > On Fri, 1 Nov 2019 13:03:13 +0000 > Emil Velikov <emil.l.velikov@gmail.com> wrote: > > > From: Emil Velikov <emil.velikov@collabora.com> > > > > As mentioned by Christian, for drivers which support only primary nodes > > this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS. > > Are you sure this is true for MODESET-only nodes (those that do not > have the RENDER cap set) implementing ->{fd_to_handle,handle_to_fd}()? > Shouldn't the is_authenticated() check still be done in that case? > Thanks for catching this. Just sent out v2, which I should address the concern. -Emil
On Wed, Nov 27, 2019 at 04:27:29PM +0000, Emil Velikov wrote: > On Wed, 27 Nov 2019 at 07:41, Boris Brezillon > <boris.brezillon@collabora.com> wrote: > > > > Hi Emil, > > > > On Fri, 1 Nov 2019 13:03:13 +0000 > > Emil Velikov <emil.l.velikov@gmail.com> wrote: > > > > > From: Emil Velikov <emil.velikov@collabora.com> > > > > > > As mentioned by Christian, for drivers which support only primary nodes > > > this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS. > > > > Are you sure this is true for MODESET-only nodes (those that do not > > have the RENDER cap set) implementing ->{fd_to_handle,handle_to_fd}()? > > Shouldn't the is_authenticated() check still be done in that case? > > > Thanks for catching this. Just sent out v2, which I should address the concern. Why do we need this additional check in v2? What can go wrong on modeset drivers if non-authenticated legacy things can use this? modeset-only drivers have all their resources segregated by the drm core (drm_fb, mmaps, buffer lists), so there's really no access limitations that can go wrong here. -Daniel
On Wed, 27 Nov 2019 at 18:04, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, Nov 27, 2019 at 04:27:29PM +0000, Emil Velikov wrote: > > On Wed, 27 Nov 2019 at 07:41, Boris Brezillon > > <boris.brezillon@collabora.com> wrote: > > > > > > Hi Emil, > > > > > > On Fri, 1 Nov 2019 13:03:13 +0000 > > > Emil Velikov <emil.l.velikov@gmail.com> wrote: > > > > > > > From: Emil Velikov <emil.velikov@collabora.com> > > > > > > > > As mentioned by Christian, for drivers which support only primary nodes > > > > this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS. > > > > > > Are you sure this is true for MODESET-only nodes (those that do not > > > have the RENDER cap set) implementing ->{fd_to_handle,handle_to_fd}()? > > > Shouldn't the is_authenticated() check still be done in that case? > > > > > Thanks for catching this. Just sent out v2, which I should address the concern. > > Why do we need this additional check in v2? What can go wrong on modeset > drivers if non-authenticated legacy things can use this? modeset-only > drivers have all their resources segregated by the drm core (drm_fb, > mmaps, buffer lists), so there's really no access limitations that can go > wrong here. Welcome back Daniel. I haven't audited the core drm code, so wasn't sure if there's any issues that may arise. Hence the conservative approach in v2. If you think this is fine as-is a formal Reviewed-by would be highly appreciated. Thanks Emil
On Wed, Nov 27, 2019 at 06:32:56PM +0000, Emil Velikov wrote: > On Wed, 27 Nov 2019 at 18:04, Daniel Vetter <daniel@ffwll.ch> wrote: > > > > On Wed, Nov 27, 2019 at 04:27:29PM +0000, Emil Velikov wrote: > > > On Wed, 27 Nov 2019 at 07:41, Boris Brezillon > > > <boris.brezillon@collabora.com> wrote: > > > > > > > > Hi Emil, > > > > > > > > On Fri, 1 Nov 2019 13:03:13 +0000 > > > > Emil Velikov <emil.l.velikov@gmail.com> wrote: > > > > > > > > > From: Emil Velikov <emil.velikov@collabora.com> > > > > > > > > > > As mentioned by Christian, for drivers which support only primary nodes > > > > > this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS. > > > > > > > > Are you sure this is true for MODESET-only nodes (those that do not > > > > have the RENDER cap set) implementing ->{fd_to_handle,handle_to_fd}()? > > > > Shouldn't the is_authenticated() check still be done in that case? > > > > > > > Thanks for catching this. Just sent out v2, which I should address the concern. > > > > Why do we need this additional check in v2? What can go wrong on modeset > > drivers if non-authenticated legacy things can use this? modeset-only > > drivers have all their resources segregated by the drm core (drm_fb, > > mmaps, buffer lists), so there's really no access limitations that can go > > wrong here. > > Welcome back Daniel. > > I haven't audited the core drm code, so wasn't sure if there's any > issues that may arise. > Hence the conservative approach in v2. > > If you think this is fine as-is a formal Reviewed-by would be highly > appreciated. I think there's a non-zero chance I'll have to eat a few hats on this, but I think v1 is solid. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Thanks > Emil
On Wed, 27 Nov 2019 at 18:37, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, Nov 27, 2019 at 06:32:56PM +0000, Emil Velikov wrote: > > On Wed, 27 Nov 2019 at 18:04, Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > On Wed, Nov 27, 2019 at 04:27:29PM +0000, Emil Velikov wrote: > > > > On Wed, 27 Nov 2019 at 07:41, Boris Brezillon > > > > <boris.brezillon@collabora.com> wrote: > > > > > > > > > > Hi Emil, > > > > > > > > > > On Fri, 1 Nov 2019 13:03:13 +0000 > > > > > Emil Velikov <emil.l.velikov@gmail.com> wrote: > > > > > > > > > > > From: Emil Velikov <emil.velikov@collabora.com> > > > > > > > > > > > > As mentioned by Christian, for drivers which support only primary nodes > > > > > > this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS. > > > > > > > > > > Are you sure this is true for MODESET-only nodes (those that do not > > > > > have the RENDER cap set) implementing ->{fd_to_handle,handle_to_fd}()? > > > > > Shouldn't the is_authenticated() check still be done in that case? > > > > > > > > > Thanks for catching this. Just sent out v2, which I should address the concern. > > > > > > Why do we need this additional check in v2? What can go wrong on modeset > > > drivers if non-authenticated legacy things can use this? modeset-only > > > drivers have all their resources segregated by the drm core (drm_fb, > > > mmaps, buffer lists), so there's really no access limitations that can go > > > wrong here. > > > > Welcome back Daniel. > > > > I haven't audited the core drm code, so wasn't sure if there's any > > issues that may arise. > > Hence the conservative approach in v2. > > > > If you think this is fine as-is a formal Reviewed-by would be highly > > appreciated. > > I think there's a non-zero chance I'll have to eat a few hats on this, but > I think v1 is solid. > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Thanks. I've just re-read the DIM instructions and pushed this to drm-misc-next. Fingers crossed, I did not butcher it this time around. -Emil
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index fcd728d7cf72..5afb39688b55 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -652,8 +652,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, 0), - DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), - DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), + DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW), + DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW), DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, 0), DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),