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

[Tvrtko Ursulin]

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