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

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

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.

/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
>>