[Intel-gfx] [PATCH 02/13] drm/i915: Introduce the i915_user_extension_method

[Chris Wilson]

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