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.
> > +
> > + 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).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
