[PATCH 4/5] drm/vmwgfx: remove custom ioctl io encoding check

[Emil Velikov]

On 2019/05/25, Thomas Hellstrom wrote:
> On Sat, 2019-05-25 at 00:39 +0200, Thomas Hellström wrote:
> > Hi, Emil
> > 
> > On Fri, 2019-05-24 at 16:26 +0100, Emil Velikov wrote:
> > > On 2019/05/24, Thomas Hellstrom wrote:
> > > > On Fri, 2019-05-24 at 13:14 +0100, Emil Velikov wrote:
> > > > > On 2019/05/23, Thomas Hellstrom wrote:
> > > > > > Hi, Emil,
> > > > > > 
> > > > > > On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
> > > > > > > From: Emil Velikov <emil.velikov at collabora.com>
> > > > > > > 
> > > > > > > Drop the custom ioctl io encoding check - core drm does it
> > > > > > > for
> > > > > > > us.
> > > > > > 
> > > > > > I fail to see where the core does this, or do I miss
> > > > > > something?
> > > > > 
> > > > > drm_ioctl() allows for the encoding to be changed and
> > > > > attributes
> > > > > that
> > > > > only the
> > > > > appropriate size is copied in/out of the kernel.
> > > > > 
> > > > > Technically the function is more relaxed relative to the vmwgfx
> > > > > check, yet
> > > > > seems perfectly reasonable.
> > > > > 
> > > > > Is there any corner-case that isn't but should be handled in
> > > > > drm_ioctl()?
> > > > 
> > > > I'd like to turn the question around and ask whether there's a
> > > > reason
> > > > we should relax the vmwgfx test? In the past it has trapped quite
> > > > a
> > > > few
> > > > user-space errors.
> > > > 
> > > The way I see it either:
> > >  - the check, as-is, is unnessesary, or
> > >  - it is needed, and we should do something equivalent for all of
> > > DRM
> > > 
> > > We had a very long brainstorming session with a colleague and we
> > > could not see
> > > any cases where this would cause a problem. If you recall anything
> > > concrete
> > > please let me know - I would be more than happy to take a closer
> > > look.
> > 
> > If you have a good reason to drop an ioctl sanity check, I'd be
> > perfectly happy to do it. To me, a good reason even includes "I have
> > a
> > non-open-source customer having problems with this check" because of
> > reason etc. etc. as long as I have a way to evaluate those reasons
> > and
> > determine if they're valid or not. "No other drm driver nor the core
> > is
> > doing this" is NOT a valid reason to me. In particular if the check
> > is
> > not affecting performance. So unless you provide additional reasons
> > to
> > drop this check, it's a solid NAK from my side.
> 
> To clarify my point of view a bit, this check is useful to early catch
> userspace using incorrect flags and sizes, which otherwise might make
> it out to distros and after that, introducing a check like this would
> be impossible, since it might break old user-space. For the same reason
> it would probably be very difficult to introduce it in core drm. 
> 
I think we might be talking past each other, let's take a step back:

 - as of previous patch, all of vmwgfx ioctls size is consistently
handled by the core
 - handling of featue flags, as always, is responsibility of the driver
ifself
 - with this patch, ioctl direction is also handled by core.

Here core ensures we only copy in/out as much data as the kernel
implementation can handle.


Let's consider the following real world example - msm and virtio_gpu.

An in field of an _IOW ioctl becomes in/out aka _IORW ioctl.
 - we add a flag to annotate/request the out, as always invalid flags
are return -EINVAL
 - we change the ioctl encoding

As currently handled by core DRM, old kernel/new userspace and
vice-versa works just fine. Sadly, vmwgfx will error out, while it could
be avoided.

As said above, I'll gladly adjust core and/or others, if this relaxed
approach causes an issue somewhere. A specific use-case, real or
hypothetical will be appreciated.


All this is part of an "evil" plan of mine, to port cool features from
vmwgfx to core and effectively remove the vmw_generic_ioctl() wrapper.


Hope the bigger picture is clearer now, if not please let me know.

Thanks
Emil