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

Mon May 27 11:34:14 UTC 2019

Hi, Emil,

On Mon, 2019-05-27 at 10:08 +0100, Emil Velikov wrote:
> 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

I don't think I follow you here, AFAICT patch 3/5 only affects and
relaxes the execbuf checking (and in fact a little more than I would
like)?

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

IMO basically we have a tradeoff between strict checking in this case,
and new user-space vs old kernel "hazzle-free" transition in the
relaxed case. 

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

To me there are two important reasons to keep the strict approach.

1) Avoid user-space mistakes early in the development cycle. We can't
distinguish between buggy user-space and "new" user-space. This is
important because of [a]) below.

2) Catch a lot of fuzzer combinations and error out early instead of
forwarding them to the ioctl function where they may cause harm.

I think the new user-space vs old kernel can be handled nicely in user-
space with feature flags or API versions. That's the way we've handled
them up to now?

[a] But you probably can't move the stricter approach to core drm, or
even to core drm ioctls, since that may break old user-space we might
not even know about. Relaxing this now may put the vmwgfx-specific
ioctls in the same situation. I'd like to have this check also in core
drm, but alas, I think it's impossible.

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

I'm not completely sure I understand what role the vmw_generic_ioctl()
wrapper plays in this? I mean, the advantage of a helper approach
instead of a middle layer approach is that you are free to use the
helpers and amend them if desirable. If everybody is forced to use
exactly the same helpers, then we're basically back at the middle layer
approach?

/Thomas

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