Quoting Tvrtko Ursulin (2019-03-13 11:13:10)
>
> 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.
I've avoided using the struct itself for markup so far.
Ugh, it would also mean that userspace has to sanitize the extension
chain between uses.
> >>> +
> >>> + 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?
Writing anything gives me the heebie-jeebies. If we keep it a read-only
struct, we can never be tricked into overwriting something important.
It also makes it harder for userspace to reuse as they have to clear the
result field?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
