On 13/03/2019 10:50, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2019-03-08 14:33:02)
On 08/03/2019 14:12, Chris Wilson wrote:
+int i915_user_extensions(struct i915_user_extension __user *ext,
+ const i915_user_extension_fn *tbl,
+ unsigned long count,
+ void *data)
+{
+ unsigned int stackdepth = 512;
I have doubts about usefulness of trying to impose some limit now. And
also reservations about using the name stack. But both are irrelevant
implementation details at this stage so meh.
We need defence against malice userspace doing
struct i915_user_extension ext = {
.next_extension = &ext,
};
so sadly some limit is required.
Oh yes, good point. I wasn't thinking maliciously enough.
S possible alternative solution could be, in conjunction with the result
field from below, to only allow visiting any extension once. It would
require reserving some value as meaning "not visited". Probably zero, so
non-zero in result would immediately fail the chain, but would also I
think mean we only support negative values in result as output, mapping
zeros to one.
+
+ while (ext) {
+ int err;
+ u64 x;
+
+ if (!stackdepth--) /* recursion vs useful flexibility */
+ return -EINVAL;
+
+ if (get_user(x, &ext->name))
+ return -EFAULT;
+
+ err = -EINVAL;
+ if (x < count && tbl[x])
+ err = tbl[x](ext, data);
How about:
put_user(err, &ext->result);
And:
struct i915_user_extension {
__u64 next_extension;
__u64 name;
__u32 result;
__u32 mbz;
};
So we add the ability for each extension to store it's exit code giving
userspace opportunity to know which one failed.
With this I would be satisfied usability is future proof enough.
I'm sorely tempted. The biggest objection I have is this defeats the
elegance of a read-only chain. So who would actually use it?
err = gem_context_create_ext(&chain);
if (err) {
struct i915_user_extension *ext = (struct i915_user_extension *)chain;
while (ext && !ext->result)
ext = (struct i915_user_extension *)ext->next_extension;
if (ext)
fprintf(stderr, "context creation failed at extension: %lld", ext->name);
}
What exactly are they going to do? They are not going to do anything
like
while (err) {
ext = first_faulty_ext(&chain);
switch (ext->name) {
case ...: do_fixup_A(ext);
}
err = gem_context_create_ext(&chain);
}
I'm not really seeing how they benefit over, and above, handling the
ioctl error by printing out the entire erroneous struct and chain, and
falling back to avoiding that ioctl.
I think what you really want is a per-application/fd debug log, so that
we can dump the actual errors as they arise (without leaking them into
the general syslog).
Maybe.. could be an extension of the existing problem of "What EINVAL
you mean exactly?" indeed.
I don't see a problem with writing back though?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
