Re: [PATCH 08/10] drm/i915/gt: Use dma_resv_alloc for VM reservation objects

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Double allocation of `dma_resv` causes a permanent memory leak because `vm->_resv` is allocated twice and the first pointer is unconditionally overwritten.
- [High] Calls to `i915_address_space_init()` ignore the new `-ENOMEM` error return value, leading to incomplete initialization and potential NULL pointer dereferences.
--

commit 5239b0e3c5a536da51a7fa10affcf28e378bea31
Author: Christian König <ckoenig.leichtzumerken@gmail.com>
drm/i915/gt: Use dma_resv_alloc for VM reservation objects

Replaces the embedded dma_resv object in struct i915_address_space with a
dynamically allocated pointer, returning int from initialization functions
to properly handle allocation failures.

> diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c b/drivers/gpu/drm/i915/gt/intel_gtt.c
> index 7b1bdb121c88d..670cbe0b8cc05 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
> @@ -258,7 +258,7 @@ void i915_vm_release(struct kref *kref)
>  	queue_work(vm->i915->wq, &vm->release_work);
>  }
>  
> -void i915_address_space_init(struct i915_address_space *vm, int subclass)
> +int i915_address_space_init(struct i915_address_space *vm, int subclass)
>  {

[Severity: High]
Does this change leave some callers unhandled? i915_address_space_init() now
returns an error code, but it appears some existing callers were not updated
to check the return value. 

For instance, i915_dpt_create() in drivers/gpu/drm/i915/i915_dpt.c, as well
as the mock tests in drivers/gpu/drm/i915/selftests/mock_gtt.c, seem to ignore
this return value. If dma_resv_alloc() fails under memory pressure, it could
leave vm->mm uninitialized and vm->_resv as NULL, which might lead to a panic
later when the Display Page Table is used.

[ ... ]

> @@ -295,7 +295,10 @@ void i915_address_space_init(struct i915_address_space *vm, int subclass)
>  		might_alloc(GFP_KERNEL);
>  		mutex_release(&vm->mutex.dep_map, _THIS_IP_);
>  	}
> -	dma_resv_init(&vm->_resv);
> +
> +	vm->_resv = dma_resv_alloc();
> +	if (!vm->_resv)
> +		return -ENOMEM;

[Severity: High]
Could this unconditionally overwrite an already allocated vm->_resv pointer?

The callers ppgtt_init() and ggtt_probe_hw() explicitly allocate vm->_resv
before calling i915_address_space_init(), which then allocates it again here.
This appears to cause a permanent memory leak of the first allocation on every
successful VM initialization.

>  
>  	GEM_BUG_ON(!vm->total);

[ ... ]

> diff --git a/drivers/gpu/drm/i915/gt/intel_ppgtt.c b/drivers/gpu/drm/i915/gt/intel_ppgtt.c
> index 72d8473a448b9..9abd4faa2e550 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ppgtt.c
> @@ -315,9 +315,17 @@ void ppgtt_init(struct i915_ppgtt *ppgtt, struct intel_gt *gt,
>  	ppgtt->vm.total = BIT_ULL(RUNTIME_INFO(i915)->ppgtt_size);
>  	ppgtt->vm.lmem_pt_obj_flags = lmem_pt_obj_flags;
>  
> -	dma_resv_init(&ppgtt->vm._resv);
> -	i915_address_space_init(&ppgtt->vm, VM_CLASS_PPGTT);
> +	ppgtt->vm._resv = dma_resv_alloc();
> +	if (!ppgtt->vm._resv)
> +		return -ENOMEM;
> +
> +	if (i915_address_space_init(&ppgtt->vm, VM_CLASS_PPGTT)) {
> +		dma_resv_put(ppgtt->vm._resv);
> +		return -ENOMEM;
> +	}

[Severity: High]
If i915_address_space_init() fails, does this leak the first allocation?

Inside i915_address_space_init(), if its own dma_resv_alloc() fails,
ppgtt->vm._resv is clobbered to NULL. When control returns here and
dma_resv_put() is called, it will be passing NULL, leaving the original
allocation leaked.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827124910.2245-1-christian.koenig@amd.com?part=8