In the error paths of reserve_gtt_with_resource() and
insert_gtt_with_resource(), a vma_res object allocated via
i915_vma_resource_alloc() was incorrectly released using kfree().

Since i915_vma_resource_alloc() allocates objects from a dedicated
kmem_cache, using kfree() instead of the corresponding
i915_vma_resource_free() causes a mismatch between allocation and
deallocation routines, potentially leading to memory corruption.

Fix this by calling i915_vma_resource_free() to properly release the
vma_res object in both functions.

Fixes: e1a4bbb6e837d ("drm/i915: Initial introduction of vma resources")
Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
---
 drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
index 7ab4c4e60264..16e72ef57bed 100644
--- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
@@ -1524,7 +1524,7 @@ static int reserve_gtt_with_resource(struct i915_vma *vma, u64 offset)
 		i915_vma_resource_init_from_vma(vma_res, vma);
 		vma->resource = vma_res;
 	} else {
-		kfree(vma_res);
+		i915_vma_resource_free(vma_res);
 	}
 	mutex_unlock(&vm->mutex);
 
@@ -1704,7 +1704,7 @@ static int insert_gtt_with_resource(struct i915_vma *vma)
 		i915_vma_resource_init_from_vma(vma_res, vma);
 		vma->resource = vma_res;
 	} else {
-		kfree(vma_res);
+		i915_vma_resource_free(vma_res);
 	}
 	mutex_unlock(&vm->mutex);
 
-- 
2.34.1

Hi Zilin,

> In the error paths of reserve_gtt_with_resource() and
> insert_gtt_with_resource(), a vma_res object allocated via
> i915_vma_resource_alloc() was incorrectly released using kfree().
> 
> Since i915_vma_resource_alloc() allocates objects from a dedicated
> kmem_cache, using kfree() instead of the corresponding
> i915_vma_resource_free() causes a mismatch between allocation and
> deallocation routines, potentially leading to memory corruption.
I would not call this "improper", because the definition of
kfree allows this usage (found in mm/slub.c):

/**
 * kfree - free previously allocated memory
 * @object: pointer returned by kmalloc() or kmem_cache_alloc()
 *
 * If @object is NULL, no operation is performed.
 */
void kfree(const void *object)
{
	struct folio *folio;
	struct slab *slab;
	struct kmem_cache *s;
	void *x = (void *)object;

	trace_kfree(_RET_IP_, object);

	if (unlikely(ZERO_OR_NULL_PTR(object)))
		return;

	folio = virt_to_folio(object);
	if (unlikely(!folio_test_slab(folio))) {
		free_large_kmalloc(folio, (void *)object);
		return;
	}

	slab = folio_slab(folio);
	s = slab->slab_cache;
	slab_free(s, slab, x, _RET_IP_);
}
EXPORT_SYMBOL(kfree);

i915_vma_resource_alloc() calls kmem_cache_zalloc, which is
defined in include/linux/slab.h:

#define kmem_cache_zalloc(_k, _flags)		kmem_cache_alloc(_k, (_flags)|__GFP_ZERO)

I understand what you are trying to do here, but I'd call this
improvement to the readability and matching alloc/free routines
rather than "fixing" it.

> 
> Fix this by calling i915_vma_resource_free() to properly release the
> vma_res object in both functions.
> 
> Fixes: e1a4bbb6e837d ("drm/i915: Initial introduction of vma resources")
I think you could drop the last sentence and Fixes tag.

> Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
> ---
>  drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> index 7ab4c4e60264..16e72ef57bed 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> @@ -1524,7 +1524,7 @@ static int reserve_gtt_with_resource(struct i915_vma *vma, u64 offset)
>  		i915_vma_resource_init_from_vma(vma_res, vma);
>  		vma->resource = vma_res;
>  	} else {
> -		kfree(vma_res);
> +		i915_vma_resource_free(vma_res);
>  	}
>  	mutex_unlock(&vm->mutex);
>  
> @@ -1704,7 +1704,7 @@ static int insert_gtt_with_resource(struct i915_vma *vma)
>  		i915_vma_resource_init_from_vma(vma_res, vma);
>  		vma->resource = vma_res;
>  	} else {
> -		kfree(vma_res);
> +		i915_vma_resource_free(vma_res);
>  	}
>  	mutex_unlock(&vm->mutex);
>  
> -- 
> 2.34.1
> 

-- 
Best Regards,
Krzysztof

On Wed, Nov 12, 2025 at 12:45:13PM +0000, Krzysztof Karas wrote:
> Hi Zilin,
> 
> > In the error paths of reserve_gtt_with_resource() and
> > insert_gtt_with_resource(), a vma_res object allocated via
> > i915_vma_resource_alloc() was incorrectly released using kfree().
> > 
> > Since i915_vma_resource_alloc() allocates objects from a dedicated
> > kmem_cache, using kfree() instead of the corresponding
> > i915_vma_resource_free() causes a mismatch between allocation and
> > deallocation routines, potentially leading to memory corruption.
> I would not call this "improper", because the definition of
> kfree allows this usage (found in mm/slub.c):
> 
> /**
>  * kfree - free previously allocated memory
>  * @object: pointer returned by kmalloc() or kmem_cache_alloc()
>  *
>  * If @object is NULL, no operation is performed.
>  */
> void kfree(const void *object)
> {
> 	struct folio *folio;
> 	struct slab *slab;
> 	struct kmem_cache *s;
> 	void *x = (void *)object;
> 
> 	trace_kfree(_RET_IP_, object);
> 
> 	if (unlikely(ZERO_OR_NULL_PTR(object)))
> 		return;
> 
> 	folio = virt_to_folio(object);
> 	if (unlikely(!folio_test_slab(folio))) {
> 		free_large_kmalloc(folio, (void *)object);
> 		return;
> 	}
> 
> 	slab = folio_slab(folio);
> 	s = slab->slab_cache;
> 	slab_free(s, slab, x, _RET_IP_);
> }
> EXPORT_SYMBOL(kfree);
> 
> i915_vma_resource_alloc() calls kmem_cache_zalloc, which is
> defined in include/linux/slab.h:
> 
> #define kmem_cache_zalloc(_k, _flags)		kmem_cache_alloc(_k, (_flags)|__GFP_ZERO)
> 
> I understand what you are trying to do here, but I'd call this
> improvement to the readability and matching alloc/free routines
> rather than "fixing" it.

Thanks for your review and for pointing out that kfree() can handle
memory from a kmem_cache. I agree that this change is better described
as an improvement for readability and to ensure matching
allocation/deallocation functions are used. I will update the commit
message accordingly in v2.

> > Fix this by calling i915_vma_resource_free() to properly release the
> > vma_res object in both functions.
> > 
> > Fixes: e1a4bbb6e837d ("drm/i915: Initial introduction of vma resources")
> I think you could drop the last sentence and Fixes tag.

I will drop the last sentence and Fixes tag in the v2 patch.

> > Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
> > ---
> >  drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> > index 7ab4c4e60264..16e72ef57bed 100644
> > --- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> > @@ -1524,7 +1524,7 @@ static int reserve_gtt_with_resource(struct i915_vma *vma, u64 offset)
> >  		i915_vma_resource_init_from_vma(vma_res, vma);
> >  		vma->resource = vma_res;
> >  	} else {
> > -		kfree(vma_res);
> > +		i915_vma_resource_free(vma_res);
> >  	}
> >  	mutex_unlock(&vm->mutex);
> >  
> > @@ -1704,7 +1704,7 @@ static int insert_gtt_with_resource(struct i915_vma *vma)
> >  		i915_vma_resource_init_from_vma(vma_res, vma);
> >  		vma->resource = vma_res;
> >  	} else {
> > -		kfree(vma_res);
> > +		i915_vma_resource_free(vma_res);
> >  	}
> >  	mutex_unlock(&vm->mutex);
> >  
> > -- 
> > 2.34.1
> > 
> 
> -- 
> Best Regards,
> Krzysztof

Best Regards,
Zilin Guan