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

[Matthew Brost]

On Fri, Jul 28, 2023 at 12:15:01PM +0200, Maarten Lankhorst wrote:
> 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.
> 

Ah, yes I see this now.

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

Thanks for explaining, makes sense now.

With that:
Reviewed-by: Matthew Brost <matthew.brost at intel.com>


> 
> ~Maarten