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

[Chris Wilson]

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