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

[Emil Velikov]

On 2019/05/27, Thomas Hellstrom wrote:
> On 5/27/19 5:27 PM, Emil Velikov wrote:
> > On 2019/05/27, Thomas Hellstrom wrote:
> > > On 5/27/19 2:35 PM, Emil Velikov wrote:
> > > > Hi Thomas,
> > > > 
> > > > On 2019/05/27, Thomas Hellstrom wrote:
> > > > 
> > > > > > 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)?
> > > > > 
> > > > Precisely, it makes execbuf ioctl behave like all other ioctls - both
> > > > vmwgfx and rest of drm.
> > > But we're still enforcing a non-relaxed size check for the other vmwgfx
> > > private ioctls, right? Which is relaxed, together with the directions, in
> > > this commit?
> > > 
> > Regardless of the patch, all !execbuf vmwgfx ioctls use the related size
> > checking from core drm.
> 
> Well it does, but since we (before this patch) enforce ioctl->cmd == cmd, we
> also enforce
> _IOC_SIZE(ioctl->cmd) == _IOC_SIZE(cmd), which makes the core check
> pointless, or am I missing something?
> 
You're spot on - I never looked at the _IOC_SIZE declaration. I was
assuming some other magic.


> > 
> > > (Not that it matters much to the discussion, though).
> > > 
> > Agreed.
> > 
> > > > 
> ...
> > > > Can you provide a concrete example, please?
> > > OK, let's say you were developing fence wait functionality. Like
> > > vmw_fence_obj_wait ioctl. Then suddenly you started to wonder why the wait
> > > never timed out as it should. The reason turn out to be that signals were
> > > restarting the waits with the original timeout. So you change the ioctl from
> > > W to RW and add a kernel-computed time to the argument. Everything is fine,
> > > except that you forget to change this in a user-space application somewhere.
> > > 
> > > So now what happens, is that that user-space bug can live on undetected as
> > > in 1), and that means you can never go back and implement a stricter check
> > > because that would completely break old user-space.
> > > 
> > If I understand you correctly, the W -> RW change in unnecessary. Yet
> > the only negative effect that I can see is the copy_to_user() overhead.
> > 
> > The copy should be negligible, yet it "feels" silly.
> > 
> > Is there anything more serious that I've missed?
> 
> Well the point is in this case, that the write was necessary, but the code
> would work sort of OK anyway. It updated a kernel "cookie" to make sure the
> timeout would be correct even with the next call repetition. Now if an old
> header was floating around, there might be clients using it. And with the
> current core checks that typically wouldn't get noticed. With the check we'd
> immediately notice and abort. It feels a little like moving from ANSI C to
> K&R... :-)
> 
Technically, the kernel (or any function really) should write output
arguments only when needed. Agree though - it's sometimes annoying or
inconvenient.

For the ANSI C to K&R comment - sure, only if one cares about backward
compat. If they don't - flip the config toggle and carry on ;-)

> > 
> > 
> > Having a closer look - vmwgfx (et al) seems to stand out, such that it
> > does not provide a UABI define including the encoding. Hence it sort of
> > duplicates that in userspace, by using the explicit drmCommand*
> > 
> > Guess I could follow the suggestion in vmwgfx_drv.c move the defines to
> > UABI, sync header and update mesa/xf86-video-vmwgfx.
> > 
> > What do you think - yes, or please don't?
> 
> Please hold on for a while, and I'll discuss it internally.
> 
Ack.

> > 
> > > The current code will trap (and has historically trapped) code like this.
> > > That's mainly why I'm reluctant to give it up, but I guess it can be
> > > conditionally compiled in for debug purposes.
> > > 
> > This piece here, is the holly grail. I'll go further and suggest:
> > 
> >   - add a strict encoding and size check, behind a config toggle
> >   - make it a core drm thing and drop the custom vmwgfx one
> > 
> > Will keep it disabled by default - but will clearly document Kconfig and
> > docs that devs should toggle it to catch bugs.
> 
> Sounds good, but IIRC the reason why I kept it only to driver-private
> ioctls, is that there were errors with the drm ioctls. But that was a long
> time ago so I might remember incorrectly, or user-space has been fixed.
> 
Good point - will have a quick look and send patches if needed.

Thanks
Emil