Re: [PATCH 3/4] drm/xe: Fix bo leak in xe_dma_buf_init_obj() on allocation failure

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Apr 08, 2026 at 09:58:06AM -0600, Lin, Shuicheng wrote:
> On Tue, Apr 7, 2026 10:01 PM Matthew Brost wrote:
> > On Tue, Apr 07, 2026 at 08:15:41PM +0000, Shuicheng Lin wrote:
> > > When drm_gpuvm_resv_object_alloc() fails, the pre-allocated storage bo
> > > is not freed. Add xe_bo_free(storage) before returning the error.
> > >
> > > Fixes: eb289a5f6cc6 ("drm/xe: Convert xe_dma_buf.c for exhaustive
> > > eviction")
> > > Cc: stable@vger.kernel.org
> > > Assisted-by: Claude:claude-opus-4.6
> > > Signed-off-by: Shuicheng Lin <shuicheng.lin@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_dma_buf.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_dma_buf.c
> > > b/drivers/gpu/drm/xe/xe_dma_buf.c index 7f9602b3363d..24d9d82426b9
> > > 100644
> > > --- a/drivers/gpu/drm/xe/xe_dma_buf.c
> > > +++ b/drivers/gpu/drm/xe/xe_dma_buf.c
> > > @@ -271,8 +271,10 @@ xe_dma_buf_init_obj(struct drm_device *dev, struct
> > xe_bo *storage,
> > >  	int ret = 0;
> > >
> > >  	dummy_obj = drm_gpuvm_resv_object_alloc(&xe->drm);
> > > -	if (!dummy_obj)
> > > +	if (!dummy_obj) {
> > 
> > I know the comment at caller says 'Errors here will take care of freeing the bo.'
> > 
> > But I'm not sure that is right sematic as this patch alone won't free the BO give
> > this line not seen in this diff:
> > 
> > 296         return ret ? ERR_PTR(ret) : &bo->ttm.base;
> > 
> > So IMO we make the caller own the freeing of the BO here.
> 
> xe_dma_buf_init_obj() calls xe_bo_init_locked(), which frees the BO on error.
> Therefore, xe_dma_buf_init_obj() must also free the BO on its error paths.

Yes, right. It is easy to forget these consuming interfaces on error.

> Otherwise, since xe_gem_prime_import() cannot distinguish whether the failure originated from xe_dma_buf_init_obj() or from xe_bo_init_locked(), it cannot safely decide whether the BO should be freed.
> 
> On success, ownership of the BO is transferred to the drm_gem_object.
> 
> How about add some comments in this function like below?
> 
> +/*
> + * Takes ownership of @storage: on success it is transferred to the returned
> + * drm_gem_object; on failure it is freed before returning the error.
> + * This matches the contract of xe_bo_init_locked() which frees @storage on
> + * its error paths, so callers need not (and must not) free @storage after
> + * this call.
> + */

Yes, that's good to avoid forgetting.

So this patch looks correct:

Reviewed-by: Matthew Brost <matthew.brost@intel.com>

>  static struct drm_gem_object *
>  xe_dma_buf_init_obj(struct drm_device *dev, struct xe_bo *storage,
>                     struct dma_buf *dma_buf)
> 
> Shuicheng
> 
> > 
> > Matt
> > 
> > > +		xe_bo_free(storage);
> > >  		return ERR_PTR(-ENOMEM);
> > > +	}
> > >
> > >  	dummy_obj->resv = resv;
> > >  	xe_validation_guard(&ctx, &xe->val, &exec, (struct xe_val_flags) {},
> > > ret) {
> > > --
> > > 2.43.0
> > >