Re: [PATCH v2] drm/syncobj: Limit the number of handles and use kvmalloc for user-sized arrays

["Christian König" <christian.koenig@amd.com>]

On 11/5/26 10:12, Tvrtko Ursulin wrote:
> On 05/11/2026 08:18, Yu, Fei wrote:
>> From: Fei Yu <fei.yu@intel.com>
>> On 04/11/2026 10:27, Tvrtko Ursulin wrote:
>>> On 04/11/2026 09:44, Christian König wrote:
>>>> On 11/4/26 04:09, Yu, Fei wrote:
>>>>> +#define DRM_SYNCOBJ_MAX_HANDLES        (INT_MAX / sizeof(struct syncobj_wait_entry))
>>>>
>>>> I really wonder if we should put that into the uAPI files?
>>>>
>>>> Not that userspace is potentially using it, but this is part of the uAPI
>>>> and makes sense for documentation purposes.
>>
>> I thought about that, but I'd prefer to keep it kernel internal, at
>> least for this fix:
>>
>>   - The value is derived from the size of a kernel internal struct and
>>     from the kvmalloc() size limit, so it is not the same on 32-bit and
>>     64-bit.

Oh that is a really good point, but most certainly a bad idea.

This means that code working on 32bit then suddenly fails on 64bit, not something we should do.

>>     To put it in drm.h I would have to replace it with a fixed
>>     number, and that number becomes ABI. We could not easily raise or
>>     lower it later.

I think I prefer a fixed number here. Something reasonable large enough, like 64k or similar, should do for backporting.

>>
>>   - Tvrtko's series below avoids most of these allocations. Once that
>>     lands, the right limit may be different, and an internal define is
>>     easier to change.
>>
>>   - The patch has a Fixes tag, and keeping it out of the uAPI header
>>     keeps the stable backport small.
>>
>> For documentation, I could send a follow-up that adds a sentence to the
>> uAPI struct comments, saying that count_handles above an implementation
>> defined limit is rejected with -EINVAL. That documents the behavior
>> without fixing a number. Would that work for you?

With Tvrko's patch to avoid most of the allocations in the future that approach sounds reasonable.

But I would still use a fixed number and maybe a static assert that we don't hit the kvmalloc limit when we allocate that fixed number of structures.

>>
>>>> That kvmalloc_array() + the following copy_from_user should probably be
>>>> replaced by using vmemdup_array_user(). But that should of course be a
>>>> separate patch.
>>>
>>> Btw I tried to do that some time ago:
>>>
>>> https://lore.kernel.org/dri-devel/20251106132645.28208-1-tvrtko.ursulin@igalia.com/#t
>>>
>>> I have as local, more recent version which applies, but apparently never
>>> sent it.
>>>
>>> Interesting part should also be the other patches from the above linked
>>> series which avoid allocations altogether for the vast majority of cases.
>>>
>>> If there is interest I can re-base and re-send after this is merged.
>>
>> Thanks, I wasn't aware of that series. I'll leave the
>> vmemdup_array_user() and points array cleanups to it, so we don't
>> duplicate work. Happy to test it once this fix is in.
> 
> Thanks - just to clarify though - I understand I wait for your patch to land and then rebase my series, right?

Right. This here is for -fixes, cleanups are for -next.

Christian.


> 
> Regards,
> 
> Tvrtko
>