[Intel-xe] [PATCH] drm/xe: Fix error paths of __xe_bo_create_locked

[Maarten Lankhorst]

Hey,

On 2023-07-25 19:22, Matthew Brost wrote:
> On Tue, Jul 25, 2023 at 05:12:39PM +0200, Maarten Lankhorst wrote:
>> ttm_bo_init_reserved() calls the destroy() callback if it fails.
>>
>> Because of this, __xe_bo_create_locked is required to be responsible
>> for freeing the bo even when it's passed in as argument.
>>
>> Additionally, if the placement check fails, the bo was kept alive.
>> Fix it too.
>>
>> Reported-by: Oded Gabbay <ogabbay at kernel.org>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_bo.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
>> index e240df6c7ae64..b63290540dd9b 100644
>> --- a/drivers/gpu/drm/xe/xe_bo.c
>> +++ b/drivers/gpu/drm/xe/xe_bo.c
>> @@ -1158,8 +1158,10 @@ struct xe_bo *__xe_bo_create_locked(struct xe_device *xe, struct xe_bo *bo,
>>   	/* Only kernel objects should set GT */
>>   	XE_BUG_ON(tile && type != ttm_bo_type_kernel);
>>   
>> -	if (XE_WARN_ON(!size))
>> +	if (XE_WARN_ON(!size)) {
>> +		xe_bo_free(bo);
>>   		return ERR_PTR(-EINVAL);
>> +	}
>>   
>>   	if (!bo) {
>>   		bo = xe_bo_alloc();
>> @@ -1197,8 +1199,10 @@ struct xe_bo *__xe_bo_create_locked(struct xe_device *xe, struct xe_bo *bo,
>>   
>>   	if (!(flags & XE_BO_FIXED_PLACEMENT_BIT)) {
>>   		err = __xe_bo_placement_for_flags(xe, bo, bo->flags);
>> -		if (WARN_ON(err))
>> +		if (WARN_ON(err)) {
>> +			xe_ttm_bo_destroy(&bo->ttm);
> 
> This doesn't look right as we haven't fully initialized the BO. An
> example of this is the snippet from xe_ttm_bo_destroy:
> 
> 1012         if (bo->vm && xe_bo_is_user(bo))
> 1013                 xe_vm_put(bo->vm);
bo->vm is only assigned later, see xe_bo_create_locked_range line 1300-1302.

> 
> This shouldn't be executed in this error path.
> 
> Also do we need to do a cleanup if ttm_bo_init_reserved fails?
No, it cleans up the bo by calling xe_ttm_bo_destroy itself on failure. 
This is also why it's safe to call xe_ttm_bo_destroy right before 
ttm_bo_init_reserved in our setup function; on failure it would have 
been called anyway.

Because of ttm_bo_init_reserved semantics, the function is slightly 
confusing, but still correct. From the ttm_bo_init_reserved docs:
"If a failure occurs, the function will call the @destroy function. 
Thus, after a failure, dereferencing @bo is illegal and will likely 
cause memory corruption."

This patch is correct, but the semantics are confusing.

~Maarten