Re: [PATCH] drm/xe/vm: Fix an error path

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On 12/20/23 17:29, Lucas De Marchi wrote:
> On Wed, Dec 20, 2023 at 05:17:49PM +0100, Thomas Hellström wrote:
>>
>> On 12/20/23 17:13, Lucas De Marchi wrote:
>>> On Wed, Dec 20, 2023 at 03:42:14PM +0100, Thomas Hellström wrote:
>>>> If using the VM_BIND_OP_UNMAP_ALL without any bound vmas for the
>>>> vm, we will end up dereferencin an uninitialized variable and leak a
>>>
>>> dereferencing
>>>
>>>> bo lock. Fix this.
>>>>
>>>> Reported-by: Dafna Hirschfeld <dhirschfeld@habana.ai>
>>>> Closes: 
>>>> https://lore.kernel.org/intel-xe/jrwua7ckbiozfcaodx4gg2h4taiuxs53j5zlpf3qzvyhyiyl2d@pbs3plurokrj/
>>>> Suggested-by: Dafna Hirschfeld <dhirschfeld@habana.ai>
>>>> Fixes: 9f232f4ae249 ("drm/xe: Port Xe to GPUVA")
>>>>
>>>
>>> ^ this newline needs to be removed so `git log --format="%(trailers)"'
>>> shows everything, not only your s-o-b.
>> Will fix these.
>>>
>>>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>>> ---
>>>> drivers/gpu/drm/xe/xe_vm.c | 8 +++++---
>>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>>>> index 1ca917b8315c..127842656a23 100644
>>>> --- a/drivers/gpu/drm/xe/xe_vm.c
>>>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>>>> @@ -2063,9 +2063,11 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, 
>>>> struct xe_bo *bo,
>>>>         if (err)
>>>>             return ERR_PTR(err);
>>>>
>>>> -        vm_bo = drm_gpuvm_bo_find(&vm->gpuvm, obj);
>>>> -        if (!vm_bo)
>>>> -            break;
>>>> +        vm_bo = drm_gpuvm_bo_obtain(&vm->gpuvm, obj);
>>>
>>> if the issue with that we don't have any bound vmas, why are we 
>>> going to
>>> create a new one just to be released?
>>
>> The expected outcome of this operation, AFAICT is, rather than to 
>> error, to create  an ops with an empty list of operations, so we 
>> could in theory kmalloc ops and just initialize its list. However 
>> that would be fragile and second-guessing what gpuvm does internally, 
>> so I opted for this solution instead.
>
> ok,
>
> Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>
>
> Adding dri-devel and Danilo to Cc as it looks like we are the first
> users of this api.
>
> The NULL vs ptr-error between _find and _obtain looked suspicious when I
> first looked, but they match the current implementation.
>
> thanks
> Lucas De Marchi

Thanks for reviewing, Lucas.

/Thomas



>
>>
>> /Thomas
>>
>>
>>
>>>
>>> Lucas De Marchi
>>>
>>>> +        if (IS_ERR(vm_bo)) {
>>>> +            xe_bo_unlock(bo);
>>>> +            return ERR_CAST(vm_bo);
>>>> +        }
>>>>
>>>>         ops = drm_gpuvm_bo_unmap_ops_create(vm_bo);
>>>>         drm_gpuvm_bo_put(vm_bo);
>>>> -- 
>>>> 2.42.0
>>>>