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 <[email protected]>
> > ---
> > 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
