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