Re: [PATCH 04/11] drm/amdgpu: rework amdgpu_userq_signal_ioctl

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

Hi Prike,

On 4/24/26 10:01, Liang, Prike wrote:
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Thursday, April 23, 2026 6:48 PM
>> To: Liang, Prike <Prike.Liang@amd.com>; Khatri, Sunil <Sunil.Khatri@amd.com>
>> Cc: Koenig, Christian <Christian.Koenig@amd.com>; Deucher, Alexander
>> <Alexander.Deucher@amd.com>; amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH 04/11] drm/amdgpu: rework amdgpu_userq_signal_ioctl
>>
>> Hi guys,
>>
>> On 4/23/26 11:58, Liang, Prike wrote:
>> ...
>>>> -static int amdgpu_userq_fence_alloc(struct amdgpu_userq_fence
>>>> **userq_fence)
>>>> +static int amdgpu_userq_fence_alloc(struct amdgpu_usermode_queue *userq,
>>>> +                                 struct amdgpu_userq_fence **pfence)
>>>>  {
>>>> -     *userq_fence = kmalloc(sizeof(**userq_fence), GFP_ATOMIC);
>>>> -     return *userq_fence ? 0 : -ENOMEM;
>>>> +     struct amdgpu_userq_fence_driver *fence_drv = userq->fence_drv;
>>>> +     struct amdgpu_userq_fence *userq_fence;
>>>> +     unsigned long count;
>>> We must initialize count; otherwise, it may contain a garbage value,
>>> which can cause amdgpu_userq_fence_alloc() to fail and, in turn, make userq
>> fence emission fail.
>>
>> I've got the same comment from both Sunil and Prike but as far as I can see  and
>> that is actually incorrect.
> This patch breaks the userq fence emit path, causing desktop boot to fail. Initializing count only works around the amdgpu_userq_fence_alloc() failure, and it doesn't address the root cause, which is that xa_find() cannot initialize count when fence_drv_xa itself hasn't been set up yet. Instead of just initializing count, we may need to check the return value of xa_find(), and if no wait fences are pending, skip retrieving the wait fence array entirely.

Yeah Sunil and I figured out what was wrong here.

I was looking at the xas_find() function and thought that xa_find() would be just a wrapper around that.

But that doesn't work like that. So I not only need to initialize count, but use the xas_fine function directly.

Thanks for pointing that out,
Christian.

> 
>>>
>>>> +     userq_fence = kmalloc(sizeof(*userq_fence), GFP_KERNEL);
>>>> +     if (!userq_fence)
>>>> +             return -ENOMEM;
>>>> +
>>>> +     /*
>>>> +      * Get the next unused entry, since we fill from the start this can be
>>>> +      * used as size to allocate the array.
>>>> +      */
>>>> +     mutex_lock(&userq->fence_drv_lock);
>>>> +     xa_find(&userq->fence_drv_xa, &count, ULONG_MAX, XA_FREE_MARK);
>>
>> The count should be initialized here. But could be that this doesn't work.
>>
>> Did you guys got a KASAN warning or something like that?
> I didn't see the KASAN warning. However, the underlying problem is that when fence_drv_xa hasn't been set up, count remains uninitialized (garbage), which eventually causes kvmalloc_array() to fail when allocating fence_drv_array.
> 
>>>> +
>>>> +     userq_fence->fence_drv_array = kvmalloc_array(count, sizeof(fence_drv),
>>>> +                                                   GFP_KERNEL);
>>>> +     if (!userq_fence->fence_drv_array) {
>>>> +             mutex_unlock(&userq->fence_drv_lock);
>>>> +             kfree(userq_fence);
>>>> +             return -ENOMEM;
>>>> +     }
>>>> +
>>>> +     userq_fence->fence_drv_array_count = count;
>>>> +     xa_extract(&userq->fence_drv_xa, (void **)userq_fence->fence_drv_array,
>>>> +                0, ULONG_MAX, count, XA_PRESENT);
>>> We may need to assign the userq_fence->fence_drv_array_count the exact copied
>> number from the xa_extract().
>>
>> Interresting point. Why could that differ ?
> Generally, xa_extract() should return the same number as count, but when there's a retry entry, the actual number of copied entries may differ from the wait fence array capacity indicated by count.
> 
>> Thanks for the comments,
>> Christian.