Re: [PATCH 08/10] drm: Forbid legacy MAP functions for DRIVER_MODESET

From: Emil Velikov <emil.l.velikov@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Ben Skeggs <bskeggs@redhat.com>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 08/10] drm: Forbid legacy MAP functions for DRIVER_MODESET
Date: Thu, 14 Apr 2016 14:57:45 +0100	[thread overview]
Message-ID: <CACvgo53LDNLJQ=A1NZ87YEqdsZv-QBQB=vrPTLUie3RjR=OnFw@mail.gmail.com> (raw)
In-Reply-To: <20160414100603.GL2510@phenom.ffwll.local>

On 14 April 2016 at 11:06, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Mar 30, 2016 at 11:39:01AM +0100, Emil Velikov wrote:
>> On 30 March 2016 at 10:45, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> > Like in
>> >
>> > commit 0e975980d435d58df2d430d688b8c18778b42218
>> > Author: Peter Antoine <peter.antoine@intel.com>
>> > Date:   Tue Jun 23 08:18:49 2015 +0100
>> >
>> >     drm: Turn off Legacy Context Functions
>> >
>> > we need to again make an exception for nouveau, but everyone else
>> > really doesn't need this.
>> >
>> > Cc: Peter Antoine <peter.antoine@intel.com>
>> > Cc: Ben Skeggs <bskeggs@redhat.com>
>> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>> > ---
>> >  drivers/gpu/drm/drm_bufs.c | 12 ++++++++++++
>> >  1 file changed, 12 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
>> > index d92db7007f62..e8a12a4fd400 100644
>> > --- a/drivers/gpu/drm/drm_bufs.c
>> > +++ b/drivers/gpu/drm/drm_bufs.c
>> > @@ -396,6 +396,10 @@ int drm_legacy_addmap_ioctl(struct drm_device *dev, void *data,
>> >         if (!(capable(CAP_SYS_ADMIN) || map->type == _DRM_AGP || map->type == _DRM_SHM))
>> >                 return -EPERM;
>> >
>> > +       if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
>> > +           drm_core_check_feature(dev, DRIVER_MODESET))
>> > +               return -EINVAL;
>> > +
>> Wondering if making this the first check in the function won't be
>> better ? We have a handful of places which preemptively check
>> DRIVER_MODESET prior to calling drm_legacy functions and similarly
>> some (last time I've looked) drm_legacy functions check for
>> DRIVER_MODESET. Perhaps we can move all the checking into the
>> drm_legacy API alone ?
>
> This is an ioctl handler, so there's not really any caller we can move
> this to.
Indeed. One could extract the legacy ioctls into separate table and/or
use a function alike drm_ioctl_permit() to generalise things. It will
allow clear and obvious separation of things, although the particular
above will make things a bit annoying.

> In general I'm split, and I just put checks like these wherever
> it makes sense, pulling them out into callers if there's an entire pile of
> them who all want the same checks.
>
> In the end I don't think it matters, as long as we dutifully combine
> drm_legacy_* with such checks, so that it's _really_ all dead code for
> modern drives.
I'd suspect that there's a few bits that are missing the check, plus
going through might be a bit time consuming. Thus thinking of a
'generic' way to handle things.

Just thinking out loud :-)

-Emil
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx