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

[Thomas Hellstrom <thellstrom@vmware.com>]

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@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. 

Thanks,
Thomas



> 
> Thanks,
> Thomas
> 
> 
> > Thanks
> > Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel