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?
(Not that it matters much to the discussion, though).
- 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.
Precisely. If I read the code correctly, ATM new userspace will fail
against old kernels. Unless userspace writes two versions of the ioctl -
with with each encoding.
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.
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.
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.
2) Catch a lot of fuzzer combinations and error out early instead of
forwarding them to the ioctl function where they may cause harm.
Struggling to see why this is a problem? At some point the fuzzer will
get past this first line of defence, so we want to make the rest of the
ioctl is robust.
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?
How is a feature flag doing to help if the encoding changes from _IOW
to _IORW?
Ah, you're referring to old user-space new kernel? Yes, I was probably
reading a bit too fast. Sorry about that.
So we're basically landing in a tradeoff between trapping problems like
above, and hazzle-free ioctl argument definition change.
OK, so I'm ok with that as long as there is a way we can compile in
strict checking, which will likely has to be as a vmwgfx-specific wrapper.
/Thomas
Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
