[PATCH 1/5] drm/xe/reg_sr: Remove register pool

[Lucas De Marchi]

On Thu, Dec 05, 2024 at 10:37:51AM -0800, Matt Roper wrote:
>On Thu, Dec 05, 2024 at 10:22:36AM -0800, Lucas De Marchi wrote:
>> That pool implementation doesn't really work: if the krealloc happens to
>> move the memory and return another address, the entries in the xarray
>> become invalid, leading to use-after-free later:
>>
>> 	BUG: KASAN: slab-use-after-free in xe_reg_sr_apply_mmio+0x570/0x760 [xe]
>> 	Read of size 4 at addr ffff8881244b2590 by task modprobe/2753
>>
>> 	Allocated by task 2753:
>> 	 kasan_save_stack+0x39/0x70
>> 	 kasan_save_track+0x14/0x40
>> 	 kasan_save_alloc_info+0x37/0x60
>> 	 __kasan_kmalloc+0xc3/0xd0
>> 	 __kmalloc_node_track_caller_noprof+0x200/0x6d0
>> 	 krealloc_noprof+0x229/0x380
>>
>> Simplify the code to fix the bug. A better pooling strategy may be added
>> back later if needed.
>>
>> Fixes: 6f9f4763bc94 ("drm/xe/reg_sr: Remove register pool")
>
>Is this line right?  It seems to be referring to a non-existent patch
>(or itself based on the title?).

uggh... wrong paste to git-cite. It was supposed to be:

Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")

thanks
Lucas De Marchi

>
>Aside from the Fixes:,
>
>        Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
>
>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_reg_sr.c       | 31 ++++++----------------------
>>  drivers/gpu/drm/xe/xe_reg_sr_types.h |  6 ------
>>  2 files changed, 6 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_reg_sr.c b/drivers/gpu/drm/xe/xe_reg_sr.c
>> index e1a0e27cda14c..c13123008e903 100644
>> --- a/drivers/gpu/drm/xe/xe_reg_sr.c
>> +++ b/drivers/gpu/drm/xe/xe_reg_sr.c
>> @@ -27,46 +27,27 @@
>>  #include "xe_reg_whitelist.h"
>>  #include "xe_rtp_types.h"
>>
>> -#define XE_REG_SR_GROW_STEP_DEFAULT	16
>> -
>>  static void reg_sr_fini(struct drm_device *drm, void *arg)
>>  {
>>  	struct xe_reg_sr *sr = arg;
>> +	struct xe_reg_sr_entry *entry;
>> +	unsigned long reg;
>> +
>> +	xa_for_each(&sr->xa, reg, entry)
>> +		kfree(entry);
>>
>>  	xa_destroy(&sr->xa);
>> -	kfree(sr->pool.arr);
>> -	memset(&sr->pool, 0, sizeof(sr->pool));
>>  }
>>
>>  int xe_reg_sr_init(struct xe_reg_sr *sr, const char *name, struct xe_device *xe)
>>  {
>>  	xa_init(&sr->xa);
>> -	memset(&sr->pool, 0, sizeof(sr->pool));
>> -	sr->pool.grow_step = XE_REG_SR_GROW_STEP_DEFAULT;
>>  	sr->name = name;
>>
>>  	return drmm_add_action_or_reset(&xe->drm, reg_sr_fini, sr);
>>  }
>>  EXPORT_SYMBOL_IF_KUNIT(xe_reg_sr_init);
>>
>> -static struct xe_reg_sr_entry *alloc_entry(struct xe_reg_sr *sr)
>> -{
>> -	if (sr->pool.used == sr->pool.allocated) {
>> -		struct xe_reg_sr_entry *arr;
>> -
>> -		arr = krealloc_array(sr->pool.arr,
>> -				     ALIGN(sr->pool.allocated + 1, sr->pool.grow_step),
>> -				     sizeof(*arr), GFP_KERNEL);
>> -		if (!arr)
>> -			return NULL;
>> -
>> -		sr->pool.arr = arr;
>> -		sr->pool.allocated += sr->pool.grow_step;
>> -	}
>> -
>> -	return &sr->pool.arr[sr->pool.used++];
>> -}
>> -
>>  static bool compatible_entries(const struct xe_reg_sr_entry *e1,
>>  			       const struct xe_reg_sr_entry *e2)
>>  {
>> @@ -112,7 +93,7 @@ int xe_reg_sr_add(struct xe_reg_sr *sr,
>>  		return 0;
>>  	}
>>
>> -	pentry = alloc_entry(sr);
>> +	pentry = kmalloc(sizeof(*pentry), GFP_KERNEL);
>>  	if (!pentry) {
>>  		ret = -ENOMEM;
>>  		goto fail;
>> diff --git a/drivers/gpu/drm/xe/xe_reg_sr_types.h b/drivers/gpu/drm/xe/xe_reg_sr_types.h
>> index ad48a52b824a1..ebe11f237fa26 100644
>> --- a/drivers/gpu/drm/xe/xe_reg_sr_types.h
>> +++ b/drivers/gpu/drm/xe/xe_reg_sr_types.h
>> @@ -20,12 +20,6 @@ struct xe_reg_sr_entry {
>>  };
>>
>>  struct xe_reg_sr {
>> -	struct {
>> -		struct xe_reg_sr_entry *arr;
>> -		unsigned int used;
>> -		unsigned int allocated;
>> -		unsigned int grow_step;
>> -	} pool;
>>  	struct xarray xa;
>>  	const char *name;
>>
>> --
>> 2.47.0
>>
>
>-- 
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation