Re: [PATCH] drm/xe/xe_migrate: Fix potential overflows expression

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3376 bytes --]


On 21-03-2024 02:27, Rodrigo Vivi wrote:
> On Wed, Mar 20, 2024 at 03:48:35PM +0530, Himal Prasad Ghimiray wrote:
>> Cast to proper datatypes to avoid overflows.
> I'm afraid that the cast wont prevent the overflow, but mask it.
> probably safer to move the multiplication to some of the helpers
> in linux/math64.h ?!
Hi Rodrigo,
Thank you for your response. The modifications are inspired by the 
inline |u64 mul_u32_u32(u32 a, u32 b)|
function defined in |linux/math64.h|. Initially, I considered using the 
same approach.
However, I discovered an architecture-specific implementation for 
|mul_u32_u32|.
To prevent ambiguity, I opted for casting, which I observed is a 
standard practice throughout Linux code.
BR
Himal
>
>> Cc: Matthew Auld<matthew.auld@intel.com>
>> Cc: Matthew Brost<matthew.brost@intel.com>
>> Cc: Rodrigo Vivi<rodrigo.vivi@intel.com>
>> Signed-off-by: Himal Prasad Ghimiray<himal.prasad.ghimiray@intel.com>
>> ---
>> These errors were highlighted by Coverity. I'm uncertain whether they
>> require attention or if it would be more appropriate to label them as
>> false positives within the tool.
>>
>> I've submitted this patch in case addressing the issues is necessary.
>> However, if reviewers determine that these issues should be marked as
>> false positives or ignored within the tool, that option is also
>> available
>>
>>   drivers/gpu/drm/xe/xe_migrate.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
>> index ee1bb938c493..2ba4fb9511f6 100644
>> --- a/drivers/gpu/drm/xe/xe_migrate.c
>> +++ b/drivers/gpu/drm/xe/xe_migrate.c
>> @@ -227,7 +227,7 @@ static int xe_migrate_prepare_vm(struct xe_tile *tile, struct xe_migrate *m,
>>   		if (vm->flags & XE_VM_FLAG_64K && level == 1)
>>   			flags = XE_PDE_64K;
>>   
>> -		entry = vm->pt_ops->pde_encode_bo(bo, map_ofs + (level - 1) *
>> +		entry = vm->pt_ops->pde_encode_bo(bo, map_ofs + (u64)(level - 1) *
>>   						  XE_PAGE_SIZE, pat_index);
>>   		xe_map_wr(xe, &bo->vmap, map_ofs + XE_PAGE_SIZE * level, u64,
>>   			  entry | flags);
>> @@ -235,7 +235,7 @@ static int xe_migrate_prepare_vm(struct xe_tile *tile, struct xe_migrate *m,
>>   
>>   	/* Write PDE's that point to our BO. */
>>   	for (i = 0; i < num_entries - num_level; i++) {
>> -		entry = vm->pt_ops->pde_encode_bo(bo, i * XE_PAGE_SIZE,
>> +		entry = vm->pt_ops->pde_encode_bo(bo, (u64)i * XE_PAGE_SIZE,
>>   						  pat_index);
>>   
>>   		xe_map_wr(xe, &bo->vmap, map_ofs + XE_PAGE_SIZE +
>> @@ -291,7 +291,7 @@ static int xe_migrate_prepare_vm(struct xe_tile *tile, struct xe_migrate *m,
>>   #define VM_SA_UPDATE_UNIT_SIZE		(XE_PAGE_SIZE / NUM_VMUSA_UNIT_PER_PAGE)
>>   #define NUM_VMUSA_WRITES_PER_UNIT	(VM_SA_UPDATE_UNIT_SIZE / sizeof(u64))
>>   	drm_suballoc_manager_init(&m->vm_update_sa,
>> -				  (map_ofs / XE_PAGE_SIZE - NUM_KERNEL_PDE) *
>> +				  (size_t)(map_ofs / XE_PAGE_SIZE - NUM_KERNEL_PDE) *
>>   				  NUM_VMUSA_UNIT_PER_PAGE, 0);
>>   
>>   	m->pt_bo = bo;
>> @@ -490,7 +490,7 @@ static void emit_pte(struct xe_migrate *m,
>>   	struct xe_vm *vm = m->q->vm;
>>   	u16 pat_index;
>>   	u32 ptes;
>> -	u64 ofs = at_pt * XE_PAGE_SIZE;
>> +	u64 ofs = (u64)at_pt * XE_PAGE_SIZE;
>>   	u64 cur_ofs;
>>   
>>   	/* Indirect access needs compression enabled uncached PAT index */
>> -- 
>> 2.25.1
>>

[-- Attachment #2: Type: text/html, Size: 14299 bytes --]