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

[Tvrtko Ursulin]

On 13/03/2019 11:21, Chris Wilson wrote:
> 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?

Yeah.. nothing then.

Shall we only reserve some space with a flags and some rsvd fields just 
in case it will need to change/grow?

Regards,

Tvrtko