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@xxxxxxxxxxxxx> > > > > > > > > > > > > > > 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
